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

[dmkhn@xxxxxxxxx]

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?

--
Denis