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

[dmukhin@xxxxxxxx]

On Thu, Jul 16, 2026 at 11:50:48AM +0200, Teddy Astie wrote:
> 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 ) ?

Whoops, thanks for the catch!

> 
> > +        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