Re: [Devel] [PATCH hci-8.1 v2 6/6] ve: add neigh_tbl failcounter

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] net: neigh_alloc() logs one overflow twice and rewords the old message
- [Low] net: neigh_tbl failcount is charged to the VE for global-limit overflows
- [Low] ve: failcount warning shares one static ratelimit state across all containers
--

commit 50b4fd033875ffa61097427a5f8a1d5372007065
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve: add neigh_tbl failcounter

This commit adds a per-VE neigh_tbl_failcount counter, exposes it in
ve.failcount, and increments it from neigh_alloc() when the neighbour
table overflows. It also changes the existing overflow message to include
the VE name.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-135520
Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c

[ ... ]

> @@ -520,8 +520,9 @@ static struct neighbour *neigh_alloc(struct neigh_table *tbl,
>  	    (glob_entries >= READ_ONCE(tbl->gc_thresh2) &&
>  	     time_after(now, READ_ONCE(tbl->last_flush) + 5 * HZ))) {
>  		if (!neigh_forced_gc(tbl, ve) && entries >= gc_thresh3) {
> -			net_info_ratelimited("%s: neighbor table overflow!\n",
> -					     tbl->id);
> +			net_info_ratelimited("%s: neighbor table overflow for ve %s!\n",
> +					     tbl->id, ve_name(ve));
> +			ve_failcount_inc(ve, neigh_tbl);

[Severity: Low]
Is the reworded net_info_ratelimited() message still needed here?

ve_failcount_inc() already expands to __ve_pr_warn_ratelimited(VE_LOG_BOTH,
...), which prints "CT%s: hits the neigh_tbl limit, see the 'neigh_tbl'
counter in ve.failcount ..." to both the host log and the VE log.

A single overflow in neigh_alloc() therefore produces two log lines from
two separate ratelimit states.

The commit message also does not mention that the existing "neighbor table
overflow!" text was changed to add the VE name.

[Severity: Low]
Is the failcount charged correctly when no per-VE counter exists?

get_perve_tbl_entries_counter() returns a per-VE counter only for AF_INET
and AF_INET6:

    switch (tbl->family) {
    case AF_INET:
            return &ve->arp_neigh_nr;
    case AF_INET6:
            return &ve->nd_neigh_nr;
    }
    return NULL;

For any other family, cnt is NULL in neigh_alloc(), so entries is set to
glob_entries and the entries >= gc_thresh3 test compares the global
tbl->gc_entries against the global threshold.

ve_failcount_inc(ve, neigh_tbl) is still called unconditionally in that
branch, so the VE owning the device is charged for a table-wide overflow.

The commit message says it counts "hits of the per-VE neigh_tbl limit".
Should the increment be limited to the case where cnt is non-NULL?

[Severity: Low]
Can one container suppress the warning for other containers here?

A container can fill its own neighbour table, for example by flooding
ARP/ND entries on its own netns, and reach this overflow branch in
neigh_alloc(). ve_failcount_inc() then calls __ve_printk_ratelimited(),
which declares one static ratelimit state per call site:

    static DEFINE_RATELIMIT_STATE(_rs,
                                  DEFAULT_RATELIMIT_INTERVAL,
                                  DEFAULT_RATELIMIT_BURST);
    if (ve_ratelimit(dst, ve, &_rs))

Because the state is shared by all VEs, a busy container could use up the
burst budget and hide the warning for other containers. The atomic
failcount itself stays correct.

>  			NEIGH_CACHE_STAT_INC(tbl, table_fulls);
>  			goto out_entries;
>  		}

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-ve-printk-irq-v2-0-db1dc4f6ed07@virtuozzo.com?part=6

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel