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

[no-reply@sashiko.virtuozzo.com]

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.

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?

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

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