Re: [PATCH v4 1/2] xen/console: correct leaky-bucket rate limiter

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

You mention "correct" in the subject, but there's no fixes tag, and
it's not clear exactly what this patch corrects.

On Wed, Jul 29, 2026 at 12:25:19AM -0700, dmukhin@xxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> Use existing 'ratelimit_ms' and 'ratelimit_burst' variables in
> do_printk_ratelimit() instead of hardcoded values 5000 and 10 respectively.
> 
> Ensure rate limiter is disabled if either 'ratelimit_ms' or 'ratelimit_burst'
> is 0.
> 
> Account for integer overflow in the rate-limiter logic.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v3:
> - fixed types
> - fixed integer division logic - I used DIM_MUL2() from xvmalloc.h
>   I hope this is fine given another pending patch which will include 
> xvmalloc.h
>   for heap allocations
> - fixed potential problem w/ overflow of toks (introduced elapsed)
> - fixed potential problem with toks == 0 which is also "uninitialized"
>   state.
> ---
>  xen/drivers/char/console.c | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index ea4e3ff34178..76a1681670c1 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -33,6 +33,7 @@
>  #include <asm/setup.h>
>  #include <xen/sections.h>
>  #include <xen/consoled.h>
> +#include <xen/xvmalloc.h>
>  
>  #ifdef CONFIG_X86
>  #include <asm/guest.h>
> @@ -1286,21 +1287,43 @@ 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 toks;
>      static unsigned long last_msg;
>      static unsigned int missed;
> +    static bool initialized;
> +    unsigned long limit;
>      unsigned long flags;
> -    unsigned long long now = NOW(); /* ns */
>      unsigned long ms;
> +    s_time_t now;
>  
> -    do_div(now, 1000000);
> -    ms = (unsigned long)now;
> +    if ( !ratelimit_ms || !ratelimit_burst )
> +        return true;
> +
> +    limit = DIM_MUL2(ratelimit_burst, ratelimit_ms);
> +
> +    now = NOW(); /* ns */
> +    do_div(now, MILLISECS(1));
> +    ms = now;
>  
>      spin_lock_irqsave(&ratelimit_lock, flags);
> -    toks += ms - last_msg;
> +
> +    if ( initialized )
> +    {
> +        unsigned long elapsed = ms - last_msg;
> +
> +        if ( toks >= limit || elapsed >= limit - toks )
> +            toks = limit;
> +        else
> +            toks += elapsed;
> +    }
> +    else
> +    {
> +        toks = limit;
> +        initialized = true;
> +    }

I'm not sure you need the `initialized` static variable.  You could
set the initial value of toks = ~0, and then if the limit is set to a
lower value it would already get adjusted as part of the toks >= limit
check?

Thanks, Roger.