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

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

On 10/2/26 23:40, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] net: neigh_alloc() drops table-specific overflow log; ARP and ND share one failcount
> --
> 
> commit 83435d15f32b1a7fcb7f3aac09fb30774f63d147
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve: add neigh_tbl failcounter
> 
> This commit adds a per-VE neigh_tbl_failcount and exposes it in
> ve.failcount. In neigh_alloc(), the "neighbor table overflow!" message
> is replaced by a call to ve_failcount_inc() when the table limit is hit.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-135520
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
>> --- a/kernel/ve/ve.c
>> +++ b/kernel/ve/ve.c
> 
> [ ... ]
> 
>> @@ -834,6 +835,7 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
>>   
>>   	atomic_set(&ve->arp_neigh_nr, 0);
>>   	atomic_set(&ve->nd_neigh_nr, 0);
>> +	atomic_set(&ve->neigh_tbl_failcount, 0);
> 
> [Severity: Medium]
> ARP and ND have separate per-VE counters and limits (arp_neigh_nr and
> nd_neigh_nr, selected by get_perve_tbl_entries_counter()), but both now
> feed the single neigh_tbl_failcount.

This is true, but IMO it won't add much value.
I'll return net_info_ratelimited message with table id and ve name
for debugging purposes. It will go to the host only.

> 
> Can the ve.failcount output tell an operator whether the ARP table or the
> ND table hit its limit?
> 
> [ ... ]
> 
>> 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,7 @@ 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);
>> +			ve_failcount_inc(ve, neigh_tbl);
>>   			NEIGH_CACHE_STAT_INC(tbl, table_fulls);
>>   			goto out_entries;
>>   		}
> 
> [Severity: Medium]
> The removed net_info_ratelimited() message named the table through
> tbl->id. The replacement message from ve_failcount_inc() is fixed text
> and has no table or family information:
> 
> "CT<name>: hits the neigh_tbl limit, see the 'neigh_tbl' counter in
> ve.failcount of the container's ve cgroup"
> 
> Is it intended to drop the arp_tbl versus nd_tbl distinction and the
> host-level overflow message? The commit message does not mention that the
> original message is being removed.
> 
> Also, this path runs for ve0 as well. Would the host then log "CT0: hits
> the neigh_tbl limit ... of the container's ve cgroup", which seems
> misleading wording for the host?
> 
> Finally, the ratelimit state in ve_failcount_inc() comes from
> __ve_pr_warn_ratelimited(), which is a single static state at the macro
> call site. Is it a concern that all VEs share it, so one container that
> overflows its table often could suppress the message for other containers?
> 

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

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