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

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

On 10/12/26 01:02, no-reply@sashiko.virtuozzo.com wrote:
> 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?

I don't see an easy solution for this.
Printing to host isn't very helpful - it will just print ratelimit
message.

Adding per-VE ratelimits is a bad idea. So yes, somebody may miss
log message if another CT is spamming them.

> 

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

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