On 01.08.2025 20:58, dm...@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 <jbeul...@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
