Re: [PATCH] console: make printk_ratelimit_{burst,ms} const

[dmkhn@xxxxxxxxx]

On Mon, Aug 04, 2025 at 09:10:34AM +0200, Jan Beulich wrote:
> On 01.08.2025 20:58, dmkhn@xxxxxxxxx wrote:
> > On Fri, Aug 01, 2025 at 09:30:34AM +0200, Jan Beulich wrote:
> >> Them not being altered by any means, their __read_mostly attribute is
> >> actually counter-productive: It causes the compiler to instantiate the
> >> variables, when already with just the attributes dropped the compiler
> >> can constant-propagate the values into the sole use site. Make the
> >> situation yet more explicit by adding const.
> >>
> >> Also switch the variables away from being plain int, and have the
> >> parameters of __printk_ratelimit() follow suit. While there also
> >> similarly adjust the type of "missed" and "lost", and - while touching
> >> the adjacent line - increase lost_str[] to accommodate any unsigned
> >> 32-bit number.
> >>
> >> Fixes: a8b1845a7845 ("Miscellaneous data placement adjustments")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> In principle {__,}printk_ratelimit() may also want to have their return
> >> type changed to bool, but I think doing so would go too far here: This
> >> would have knock-on effects elsewhere, and it would want considering to
> >> actually flip polarity.
> >>
> >> Despite the Fixes: tag I wouldn't consider this for backport.
> >>
> >> --- a/xen/drivers/char/console.c
> >> +++ b/xen/drivers/char/console.c
> >> @@ -1268,12 +1268,12 @@ void console_end_sync(void)
> >>   * This enforces a rate limit: not more than one kernel message
> >>   * every printk_ratelimit_ms (millisecs).
> >>   */
> >> -int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst)
> >> +int __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 int missed;
> >> +    static unsigned int missed;
> >>      unsigned long flags;
> >>      unsigned long long now = NOW(); /* ns */
> >>      unsigned long ms;
> >> @@ -1288,14 +1288,16 @@ int __printk_ratelimit(int ratelimit_ms,
> >>          toks = ratelimit_burst * ratelimit_ms;
> >>      if ( toks >= ratelimit_ms )
> >>      {
> >> -        int lost = missed;
> >> +        unsigned int lost = missed;
> >> +
> >>          missed = 0;
> >>          toks -= ratelimit_ms;
> >>          spin_unlock(&ratelimit_lock);
> >>          if ( lost )
> >>          {
> >> -            char lost_str[8];
> >> -            snprintf(lost_str, sizeof(lost_str), "%d", lost);
> >> +            char lost_str[10];
> >> +
> >> +            snprintf(lost_str, sizeof(lost_str), "%u", lost);
> >
> > Since this code is touched, I would also simplify the entire `if ( lost )`
> > block (I have it done in another experiment):
> >
> >             char lost_str[64];
> >             size_t lost_len = snprintf(lost_str, sizeof(lost_str),
> >                                        "printk: %d messages suppressed.\n",
> >                                        lost_str);
> >
> >             /* console_lock may already be acquired by printk(). */
> >             rspin_lock(&console_lock);
> >             printk_start_of_line(CONSOLE_PREFIX, cflags);
> >             __putstr(lost_str, lost_len);
> >             ...
> >
> > What do you think?
> 
> Maybe, but definitely not right in this patch.

Sure, please consider

Reviewed-by: Denis Mukhin <dmukhin@xxxxxxxx>