Re: [Devel] [PATCH hci-8.1 v3 3/6] ve/printk: Specify ve in printks

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] ve/ratelimit: global net_ratelimit_state mixes suppressed counts across VEs
--

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

ve/printk: Specify ve in printks

This commit adds ve_printk helpers that take an explicit VE pointer, so
that printing and ratelimiting no longer depend on get_exec_env() where it
is unreliable. The VE requirement is forwarded through ve_net_ratelimit()
and the net_velog_ratelimited()/net_veboth_ratelimited() macros, and the
netfilter callers now pass net->owner_ve.

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

> diff --git a/lib/ratelimit.c b/lib/ratelimit.c
> --- a/lib/ratelimit.c
> +++ b/lib/ratelimit.c

[ ... ]

> @@ -55,7 +56,7 @@ int __ve_ratelimit(int dst, struct ve_struct *ve, struct ratelimit_state *rs, const char *func)
>  
>  		if (m) {
>  			if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
> -				ve_printk_deferred(dst, KERN_WARNING
> +				ve_printk_deferred(dst, ve, KERN_WARNING
>  						"%s: %d callbacks suppressed\n", func, m);
>  			}
>  		}

[Severity: Low]
This isn't a regression in the new interface, but the miss count printed
here comes from the shared ratelimit_state, so it includes suppressions
caused by every VE. Is it intended that the aggregated count is written
only into the log of whichever VE happens to reset the interval?

The shared state is defined once in net/core/utils.c:

net/core/utils.c:
    DEFINE_RATELIMIT_STATE(net_ratelimit_state, 5 * HZ, 10);
    ...
    int ve_net_ratelimit(int dst, struct ve_struct *ve)
    {
            return ve_ratelimit(dst, ve, &net_ratelimit_state);
    }

For example, if a container fills its own conntrack table and triggers
net_veboth_ratelimited() in __nf_conntrack_alloc(), the "N callbacks
suppressed" notice in another VE's log can include misses from the first
VE. A busy VE can also consume the shared burst of 10 per 5 seconds and
suppress the netfilter warnings of other VEs.

This patch now makes the target VE explicit, but the ratelimit_state
remains global. Would a per-VE ratelimit_state be needed for the ve
argument to give per-VE behaviour?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012-ve-printk-irq-v3-0-efc95fd5c78a@virtuozzo.com?part=3

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