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

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

[Jan Beulich]

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@suse.com>
---
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);
             /* console_lock may already be acquired by printk(). */
             rspin_lock(&console_lock);
             printk_start_of_line(CONSOLE_PREFIX);
@@ -1312,11 +1314,11 @@ int __printk_ratelimit(int ratelimit_ms,
     return 0;
 }
 
-/* minimum time in ms between messages */
-static int __read_mostly printk_ratelimit_ms = 5 * 1000;
+/* Minimum time in ms between messages */
+static const unsigned int printk_ratelimit_ms = 5 * 1000;
 
-/* number of messages we send before ratelimiting */
-static int __read_mostly printk_ratelimit_burst = 10;
+/* Number of messages we send before ratelimiting */
+static const unsigned int printk_ratelimit_burst = 10;
 
 int printk_ratelimit(void)
 {
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -80,7 +80,8 @@ extern void guest_printk(const struct do
     __attribute__ ((format (printf, 2, 3)));
 extern void noreturn panic(const char *fmt, ...)
     __attribute__ ((format (printf, 1, 2)));
-extern int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst);
+extern int __printk_ratelimit(unsigned int ratelimit_ms,
+                              unsigned int ratelimit_burst);
 extern int printk_ratelimit(void);
 
 #define gprintk(lvl, fmt, args...) \

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

[dmkhn]

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@suse.com>
> ---
> 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

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

[Jan Beulich]

On 01.08.2025 20:58, dmkhn@proton.me 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@suse.com>
>> ---
>> 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.

Jan

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

[dmkhn]

On Mon, Aug 04, 2025 at 09:10:34AM +0200, Jan Beulich wrote:
> On 01.08.2025 20:58, dmkhn@proton.me 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@suse.com>
> >> ---
> >> 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@ford.com> 

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

[Jason Andryuk]

On 2025-08-01 03:30, 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@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>