Re: [PATCH v3 2/4] xen/console: correct leaky-bucket rate limiter

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 15/07/2026 à 22:24, dmukhin@xxxxxxxx a écrit :
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Use existing printk_ratelimit_ms and printk_ratelimit_burst variables in
> do_printk_ratelimit() instead of hardcoded values 5000 and 10 respectively.
>
> Ensure rate limiter is disabled if either printk_ratelimit_ms or
> printk_ratelimit_burst is 0. Make sure no unnecessary initialization is done
> in the corner case.
>
> Also, simplify the limiter code by using min().
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v2:
> - fixed typing and 32-bit integer overflow problem
> ---
>   xen/drivers/char/console.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 5d395f882e08..de9f2432445d 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -1274,21 +1274,26 @@ bool __printk_ratelimit(unsigned int ratelimit_ms,
>                           unsigned int ratelimit_burst)
>   {
>       static DEFINE_SPINLOCK(ratelimit_lock);
> -    static unsigned long toks = 10 * 5 * 1000;
> -    static unsigned long last_msg;
> +    static unsigned long long toks, last_msg;
>       static unsigned int missed;
> +    unsigned long long now, limit;
>       unsigned long flags;
> -    unsigned long long now = NOW(); /* ns */
> -    unsigned long ms;
> +    s_time_t ms;
>   
> -    do_div(now, 1000000);
> -    ms = (unsigned long)now;
> +    if ( !ratelimit_burst || !ratelimit_burst )
Do you intend here ( !ratelimit_ms || !ratelimit_burst ) ?

> +        return true;
> +
> +    limit = min(ratelimit_burst * ratelimit_ms, UINT_MAX);
That looks no-op (at least at first stance), as UINT_MAX is the largest 
unsigned int value; hence `ratelimit_burst * ratelimit_ms` (both 
unsigned int) can't be larger than it; even if it overflows.
I think we need to cast both ratelimit_ms and ratelimit_burst to 
unsigned long long before doing the multiply, so that the multiply can't 
overflow (not sure exactly how to write it without being too verbose 
though).
https://godbolt.org/z/sxWaPM3cr

> +    if ( !toks )
> +        toks = limit;
> +
> +    now = NOW(); /* ns */
> +    ms = do_div(now, MILLISECS(1));
>   
>       spin_lock_irqsave(&ratelimit_lock, flags);
>       toks += ms - last_msg;
>       last_msg = ms;
> -    if ( toks > (ratelimit_burst * ratelimit_ms))
> -        toks = ratelimit_burst * ratelimit_ms;
> +    toks = min(toks, limit);
>       if ( toks >= ratelimit_ms )
>       {
>           unsigned int lost = missed;
With the if part fixed and the adjustments in the limit computation 
(cast to unsigned long long before the multiply) :
Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature