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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan