[PATCH v1] xen/console: remove __printk_ratelimit()

[PATCH v1] xen/console: remove __printk_ratelimit()

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com> 

__printk_ratelimit() is never used outside of the console driver.
Remove it from the lib.h and merge with the public printk_ratelimit().

Not a functional change.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
 xen/drivers/char/console.c | 29 ++++++++++++-----------------
 xen/include/xen/lib.h      |  1 -
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 75fa033ce74d..80f8f2ed1bae 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -979,7 +979,7 @@ static void vprintk_common(const char *fmt, va_list args, const char *prefix)
     char         *p, *q;
     unsigned long flags;
 
-    /* console_lock can be acquired recursively from __printk_ratelimit(). */
+    /* console_lock can be acquired recursively from printk_ratelimit(). */
     local_irq_save(flags);
     rspin_lock(&console_lock);
     state = &this_cpu(state);
@@ -1266,13 +1266,19 @@ void console_end_sync(void)
     atomic_dec(&print_everything);
 }
 
+/* minimum time in ms between messages */
+static int __read_mostly printk_ratelimit_ms = 5 * 1000;
+
+/* number of messages we send before ratelimiting */
+static int __read_mostly printk_ratelimit_burst = 10;
+
 /*
  * printk rate limiting, lifted from Linux.
  *
  * 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(void)
 {
     static DEFINE_SPINLOCK(ratelimit_lock);
     static unsigned long toks = 10 * 5 * 1000;
@@ -1288,13 +1294,13 @@ int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst)
     spin_lock_irqsave(&ratelimit_lock, flags);
     toks += ms - last_msg;
     last_msg = ms;
-    if ( toks > (ratelimit_burst * ratelimit_ms))
-        toks = ratelimit_burst * ratelimit_ms;
-    if ( toks >= ratelimit_ms )
+    if ( toks > (printk_ratelimit_burst * printk_ratelimit_ms) )
+        toks = printk_ratelimit_burst * printk_ratelimit_ms;
+    if ( toks >= printk_ratelimit_ms )
     {
         int lost = missed;
         missed = 0;
-        toks -= ratelimit_ms;
+        toks -= printk_ratelimit_ms;
         spin_unlock(&ratelimit_lock);
         if ( lost )
         {
@@ -1316,17 +1322,6 @@ int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst)
     return 0;
 }
 
-/* minimum time in ms between messages */
-static int __read_mostly printk_ratelimit_ms = 5 * 1000;
-
-/* number of messages we send before ratelimiting */
-static int __read_mostly printk_ratelimit_burst = 10;
-
-int printk_ratelimit(void)
-{
-    return __printk_ratelimit(printk_ratelimit_ms, printk_ratelimit_burst);
-}
-
 /*
  * **************************************************************
  * ********************** Error-report **************************
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index e63ec5039f92..a33c25dd1610 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -80,7 +80,6 @@ extern void guest_printk(const struct domain *d, const char *fmt, ...)
     __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(void);
 
 #define gprintk(lvl, fmt, args...) \
-- 
2.34.1

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[Julien Grall]

Hi Denis,

On 25/07/2025 22:24, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> __printk_ratelimit() is never used outside of the console driver.
> Remove it from the lib.h and merge with the public printk_ratelimit().

Is this solving any sort of violation? Asking because even if the 
function is only used by one caller, I could see a benefit to be able to 
use different value for the ratelimit. So I leaning towards keep the 
code as-is.

Cheers,

-- 
Julien Grall

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[dmkhn]

On Sat, Jul 26, 2025 at 10:20:58AM +0100, Julien Grall wrote:

Hi Julien,

Thanks for your feedback!

I imagined that kind of a change may raise a question about importance of
doing it.

> Hi Denis,
> 
> On 25/07/2025 22:24, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > __printk_ratelimit() is never used outside of the console driver.
> > Remove it from the lib.h and merge with the public printk_ratelimit().
> 
> Is this solving any sort of violation? Asking because even if the
> function is only used by one caller, I could see a benefit to be able to
> use different value for the ratelimit. So I leaning towards keep the
> code as-is.

The change is not solving any sort of violation, but simplifies a code path
slightly since fine-grain rate-limit controls are not exported/used anywhere
outside of the console driver. At this time, the purpose is a tiny cleanup.

TL;DR

I stepped over that code during addressing feedback for another change for
domain prefixes, where I'm experimenting with re-using printk-facilities from
guest_console_write(). This code showed up wrt rate-limiting for dom0's
messages. Just in case, experimental branch is here:
   https://gitlab.com/xen-project/people/dmukhin/xen/-/tree/console-fixes

I cannot anticipate not exposing rate-limiting timing controls in embedded
certifiable setup, perhaps there will be a legit use case in production
deployment.

But currently rate-limiting configuration happens at boot time only. The fact
that rate-limiting has not been touched since its introduction in 2006
    26cf03554a75 ("[XEN] Implement rate-limited logging.")
made me think that there's no intent to plumb any rate-limiting controls at
the run-time e.g. via keyhandler or a even hypercall.

Which resulted in this submission.

> 
> Cheers,
> 
> --
> Julien Grall
> 

Thanks,
Denis

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[Jan Beulich]

On 26.07.2025 11:20, Julien Grall wrote:
> On 25/07/2025 22:24, dmkhn@proton.me wrote:
>> From: Denis Mukhin <dmukhin@ford.com>
>>
>> __printk_ratelimit() is never used outside of the console driver.
>> Remove it from the lib.h and merge with the public printk_ratelimit().
> 
> Is this solving any sort of violation? Asking because even if the 
> function is only used by one caller, I could see a benefit to be able to 
> use different value for the ratelimit. So I leaning towards keep the 
> code as-is.

+1

In fact I'm surprised (or maybe not) that we still don't make better use
the rate limiting functionality.

Jan

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[dmkhn]

On Mon, Jul 28, 2025 at 11:32:43AM +0200, Jan Beulich wrote:
> On 26.07.2025 11:20, Julien Grall wrote:
> > On 25/07/2025 22:24, dmkhn@proton.me wrote:
> >> From: Denis Mukhin <dmukhin@ford.com>
> >>
> >> __printk_ratelimit() is never used outside of the console driver.
> >> Remove it from the lib.h and merge with the public printk_ratelimit().
> >
> > Is this solving any sort of violation? Asking because even if the
> > function is only used by one caller, I could see a benefit to be able to
> > use different value for the ratelimit. So I leaning towards keep the
> > code as-is.
> 
> +1
> 
> In fact I'm surprised (or maybe not) that we still don't make better use
> the rate limiting functionality.

Out of curiosity, do you have any ideas re: make better use of the rate
limiting functionality?

Build-time parameterization?

> 
> Jan
> 

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[Jan Beulich]

On 30.07.2025 00:18, dmkhn@proton.me wrote:
> On Mon, Jul 28, 2025 at 11:32:43AM +0200, Jan Beulich wrote:
>> On 26.07.2025 11:20, Julien Grall wrote:
>>> On 25/07/2025 22:24, dmkhn@proton.me wrote:
>>>> From: Denis Mukhin <dmukhin@ford.com>
>>>>
>>>> __printk_ratelimit() is never used outside of the console driver.
>>>> Remove it from the lib.h and merge with the public printk_ratelimit().
>>>
>>> Is this solving any sort of violation? Asking because even if the
>>> function is only used by one caller, I could see a benefit to be able to
>>> use different value for the ratelimit. So I leaning towards keep the
>>> code as-is.
>>
>> +1
>>
>> In fact I'm surprised (or maybe not) that we still don't make better use
>> the rate limiting functionality.
> 
> Out of curiosity, do you have any ideas re: make better use of the rate
> limiting functionality?

No concrete ones; thinking about this has been way too long ago.

> Build-time parameterization?

That and/or command line controls.

Jan

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[dmkhn]

On Wed, Jul 30, 2025 at 07:35:04AM +0200, Jan Beulich wrote:
> On 30.07.2025 00:18, dmkhn@proton.me wrote:
> > On Mon, Jul 28, 2025 at 11:32:43AM +0200, Jan Beulich wrote:
> >> On 26.07.2025 11:20, Julien Grall wrote:
> >>> On 25/07/2025 22:24, dmkhn@proton.me wrote:
> >>>> From: Denis Mukhin <dmukhin@ford.com>
> >>>>
> >>>> __printk_ratelimit() is never used outside of the console driver.
> >>>> Remove it from the lib.h and merge with the public printk_ratelimit().
> >>>
> >>> Is this solving any sort of violation? Asking because even if the
> >>> function is only used by one caller, I could see a benefit to be able to
> >>> use different value for the ratelimit. So I leaning towards keep the
> >>> code as-is.
> >>
> >> +1
> >>
> >> In fact I'm surprised (or maybe not) that we still don't make better use
> >> the rate limiting functionality.
> >
> > Out of curiosity, do you have any ideas re: make better use of the rate
> > limiting functionality?
> 
> No concrete ones; thinking about this has been way too long ago.
> 
> > Build-time parameterization?
> 
> That and/or command line controls.

Got it.

Can you please explain why exporting __printk_ratelimit() is still required
for implementation of build/command line settings in console.c?

> 
> Jan

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[Jan Beulich]

On 30.07.2025 20:06, dmkhn@proton.me wrote:
> On Wed, Jul 30, 2025 at 07:35:04AM +0200, Jan Beulich wrote:
>> On 30.07.2025 00:18, dmkhn@proton.me wrote:
>>> On Mon, Jul 28, 2025 at 11:32:43AM +0200, Jan Beulich wrote:
>>>> On 26.07.2025 11:20, Julien Grall wrote:
>>>>> On 25/07/2025 22:24, dmkhn@proton.me wrote:
>>>>>> From: Denis Mukhin <dmukhin@ford.com>
>>>>>>
>>>>>> __printk_ratelimit() is never used outside of the console driver.
>>>>>> Remove it from the lib.h and merge with the public printk_ratelimit().
>>>>>
>>>>> Is this solving any sort of violation? Asking because even if the
>>>>> function is only used by one caller, I could see a benefit to be able to
>>>>> use different value for the ratelimit. So I leaning towards keep the
>>>>> code as-is.
>>>>
>>>> +1
>>>>
>>>> In fact I'm surprised (or maybe not) that we still don't make better use
>>>> the rate limiting functionality.
>>>
>>> Out of curiosity, do you have any ideas re: make better use of the rate
>>> limiting functionality?
>>
>> No concrete ones; thinking about this has been way too long ago.
>>
>>> Build-time parameterization?
>>
>> That and/or command line controls.
> 
> Got it.
> 
> Can you please explain why exporting __printk_ratelimit() is still required
> for implementation of build/command line settings in console.c?

I didn't say console.c, did I? Whatever subsystem wanted to do proper rate
limiting would need to gain some way of controlling this (as said, build
time or cmdline driven), and it'll then need that function: How would it
effect the behavior with custom ms and/or burst values, without having
that function to call? (It is another thing that the function, using static
variables rather than per-caller state, may not be quite ready for that
kind of use. Also the arbitrary and hard-coded 10 * 5 * 1000 there would
probably also want to be customizable.)

What you may want to do for Misra's sake is make the function static for
the time being. The compiler will then fold it into its sole caller,
until some new user appears. (At that occasion dropping one of the
underscores may also be reasonable.)

Jan

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[dmkhn]

Hi Jan,

Thanks for the feedback.

On Thu, Jul 31, 2025 at 08:23:16AM +0200, Jan Beulich wrote:
> On 30.07.2025 20:06, dmkhn@proton.me wrote:
> > On Wed, Jul 30, 2025 at 07:35:04AM +0200, Jan Beulich wrote:
> >> On 30.07.2025 00:18, dmkhn@proton.me wrote:
> >>> On Mon, Jul 28, 2025 at 11:32:43AM +0200, Jan Beulich wrote:
> >>>> On 26.07.2025 11:20, Julien Grall wrote:
> >>>>> On 25/07/2025 22:24, dmkhn@proton.me wrote:
> >>>>>> From: Denis Mukhin <dmukhin@ford.com>
> >>>>>>
> >>>>>> __printk_ratelimit() is never used outside of the console driver.
> >>>>>> Remove it from the lib.h and merge with the public printk_ratelimit().
> >>>>>
> >>>>> Is this solving any sort of violation? Asking because even if the
> >>>>> function is only used by one caller, I could see a benefit to be able to
> >>>>> use different value for the ratelimit. So I leaning towards keep the
> >>>>> code as-is.
> >>>>
> >>>> +1
> >>>>
> >>>> In fact I'm surprised (or maybe not) that we still don't make better use
> >>>> the rate limiting functionality.
> >>>
> >>> Out of curiosity, do you have any ideas re: make better use of the rate
> >>> limiting functionality?
> >>
> >> No concrete ones; thinking about this has been way too long ago.
> >>
> >>> Build-time parameterization?
> >>
> >> That and/or command line controls.
> >
> > Got it.
> >
> > Can you please explain why exporting __printk_ratelimit() is still required
> > for implementation of build/command line settings in console.c?
> 
> I didn't say console.c, did I? Whatever subsystem wanted to do proper rate

But you also did not say anything about idea of having per-subsystem rate
limiting.

> limiting would need to gain some way of controlling this (as said, build
> time or cmdline driven), and it'll then need that function: How would it
> effect the behavior with custom ms and/or burst values, without having
> that function to call? (It is another thing that the function, using static
> variables rather than per-caller state, may not be quite ready for that
> kind of use. Also the arbitrary and hard-coded 10 * 5 * 1000 there would
> probably also want to be customizable.)
> 
> What you may want to do for Misra's sake is make the function static for
> the time being. The compiler will then fold it into its sole caller,
> until some new user appears. (At that occasion dropping one of the
> underscores may also be reasonable.)

Do I understand it correctly that you will accept the following submission:
 1) make __printk_ratelimit() static
 2) drop one underscore from the name
 3) keep the only call __printk_ratelimit() in a hope of there will be
    per-subsystem rate limiting in the future?

--
Denis

> 
> Jan
> 

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[Jan Beulich]

On 31.07.2025 23:28, dmkhn@proton.me wrote:
> On Thu, Jul 31, 2025 at 08:23:16AM +0200, Jan Beulich wrote:
>> On 30.07.2025 20:06, dmkhn@proton.me wrote:
>>> On Wed, Jul 30, 2025 at 07:35:04AM +0200, Jan Beulich wrote:
>>>> On 30.07.2025 00:18, dmkhn@proton.me wrote:
>>>>> On Mon, Jul 28, 2025 at 11:32:43AM +0200, Jan Beulich wrote:
>>>>>> On 26.07.2025 11:20, Julien Grall wrote:
>>>>>>> On 25/07/2025 22:24, dmkhn@proton.me wrote:
>>>>>>>> From: Denis Mukhin <dmukhin@ford.com>
>>>>>>>>
>>>>>>>> __printk_ratelimit() is never used outside of the console driver.
>>>>>>>> Remove it from the lib.h and merge with the public printk_ratelimit().
>>>>>>>
>>>>>>> Is this solving any sort of violation? Asking because even if the
>>>>>>> function is only used by one caller, I could see a benefit to be able to
>>>>>>> use different value for the ratelimit. So I leaning towards keep the
>>>>>>> code as-is.
>>>>>>
>>>>>> +1
>>>>>>
>>>>>> In fact I'm surprised (or maybe not) that we still don't make better use
>>>>>> the rate limiting functionality.
>>>>>
>>>>> Out of curiosity, do you have any ideas re: make better use of the rate
>>>>> limiting functionality?
>>>>
>>>> No concrete ones; thinking about this has been way too long ago.
>>>>
>>>>> Build-time parameterization?
>>>>
>>>> That and/or command line controls.
>>>
>>> Got it.
>>>
>>> Can you please explain why exporting __printk_ratelimit() is still required
>>> for implementation of build/command line settings in console.c?
>>
>> I didn't say console.c, did I? Whatever subsystem wanted to do proper rate
> 
> But you also did not say anything about idea of having per-subsystem rate
> limiting.
> 
>> limiting would need to gain some way of controlling this (as said, build
>> time or cmdline driven), and it'll then need that function: How would it
>> effect the behavior with custom ms and/or burst values, without having
>> that function to call? (It is another thing that the function, using static
>> variables rather than per-caller state, may not be quite ready for that
>> kind of use. Also the arbitrary and hard-coded 10 * 5 * 1000 there would
>> probably also want to be customizable.)
>>
>> What you may want to do for Misra's sake is make the function static for
>> the time being. The compiler will then fold it into its sole caller,
>> until some new user appears. (At that occasion dropping one of the
>> underscores may also be reasonable.)
> 
> Do I understand it correctly that you will accept the following submission:
>  1) make __printk_ratelimit() static
>  2) drop one underscore from the name

Yes, if you really think that's worth it.

Jan

>  3) keep the only call __printk_ratelimit() in a hope of there will be
>     per-subsystem rate limiting in the future?
> 
> --
> Denis
> 
>>
>> Jan
>>
> 

Re: [PATCH v1] xen/console: remove __printk_ratelimit()

[Roger Pau Monné]

On Fri, Jul 25, 2025 at 09:24:48PM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
> 
> __printk_ratelimit() is never used outside of the console driver.
> Remove it from the lib.h and merge with the public printk_ratelimit().
> 
> Not a functional change.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
>  xen/drivers/char/console.c | 29 ++++++++++++-----------------
>  xen/include/xen/lib.h      |  1 -
>  2 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 75fa033ce74d..80f8f2ed1bae 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -979,7 +979,7 @@ static void vprintk_common(const char *fmt, va_list args, const char *prefix)
>      char         *p, *q;
>      unsigned long flags;
>  
> -    /* console_lock can be acquired recursively from __printk_ratelimit(). */
> +    /* console_lock can be acquired recursively from printk_ratelimit(). */
>      local_irq_save(flags);
>      rspin_lock(&console_lock);
>      state = &this_cpu(state);
> @@ -1266,13 +1266,19 @@ void console_end_sync(void)
>      atomic_dec(&print_everything);
>  }
>  
> +/* minimum time in ms between messages */
> +static int __read_mostly printk_ratelimit_ms = 5 * 1000;
> +
> +/* number of messages we send before ratelimiting */
> +static int __read_mostly printk_ratelimit_burst = 10;

You possibly want to make them const for the time being?

It's unclear whether we would like to dynamically change those values
in the future.  Given the current usage they would better be const.

> +
>  /*
>   * printk rate limiting, lifted from Linux.
>   *
>   * 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(void)

As suggested by Jan, I would just make this static and remove the
prototype from lib.h.  That results in less changes, and allows to
more easily export the fine grained function in the future if we had a
need to.

Thanks, Roger.