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

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

On 10/7/26 00:06, no-reply@sashiko.virtuozzo.com wrote:
> 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.

Kept old message to print table id. You asked in v1.

> 
> [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?

As said in v1 reply, limit is global and doesn't depend on the type.
The code there is already ugly, no point in making it more complex.

> 
> [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;
>>   		}

Yes, it can. We don't have per-container ratelimits, they won't work.

> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

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