2020-11-26 21:11:59

by Paul Gortmaker

[permalink] [raw]
Subject: [PATCH 0/3] clear_warn_once: add timed interval resetting

The existing clear_warn_once functionality is currently a manually
issued state reset via the file /sys/kernel/debug/clear_warn_once when
debugfs is mounted. The idea being that a developer would be running
some tests, like LTP or similar, and want to check reproducibility
without having to reboot.

But you currently can't make use of clear_warn_once unless you've got
debugfs enabled and mounted - which may not be desired by some people
in some deployment situations.

The functionality added here allows for periodic resets in addition to
the one-shot reset it already had. Then we allow for a boot-time setting
of the periodic resets so it can be used even when debugfs isn't mounted.

By having a periodic reset, we also open the door for having the various
"once" functions act as long period ratelimited messages, where a sysadmin
can pick an hour or a day reset if they are facing an issue and are
wondering "did this just happen once, or am I only being informed once?"

Tested with DEBUG_FS_ALLOW_ALL and DEBUG_FS_ALLOW_NONE on an otherwise
defconfig.

---

Cc: Andi Kleen <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: John Ogness <[email protected]>

Paul Gortmaker (3):
clear_warn_once: expand debugfs to include read support
clear_warn_once: bind a timer to written reset value
clear_warn_once: add a warn_once_reset= boot parameter

.../admin-guide/clearing-warn-once.rst | 9 +++
.../admin-guide/kernel-parameters.txt | 8 ++
kernel/panic.c | 78 +++++++++++++++++--
3 files changed, 90 insertions(+), 5 deletions(-)

--
2.25.1


2020-11-26 23:27:50

by Paul Gortmaker

[permalink] [raw]
Subject: [PATCH 3/3] clear_warn_once: add a warn_once_reset= boot parameter

In the event debugfs is not mounted, or if built with the .config
setting DEBUG_FS_ALLOW_NONE chosen, this gives the sysadmin access
to reset the WARN_ONCE() state on a periodic basis.

Cc: Andi Kleen <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: John Ogness <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
.../admin-guide/kernel-parameters.txt | 8 +++++++
kernel/panic.c | 21 +++++++++++++++++++
2 files changed, 29 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 44fde25bb221..89f54444fee7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5863,6 +5863,14 @@
vt.underline= [VT] Default color for underlined text; 0-15.
Default: 3 = cyan.

+ warn_once_reset=
+ [KNL]
+ Set the WARN_ONCE reset period in seconds. Normally
+ a WARN_ONCE() will only ever emit a message once per
+ boot, but for example, setting this to 3600 would
+ effectively rate-limit WARN_ONCE to once per hour.
+ Default: 0 = never.
+
watchdog timers [HW,WDT] For information on watchdog timers,
see Documentation/watchdog/watchdog-parameters.rst
or other driver-specific files in the
diff --git a/kernel/panic.c b/kernel/panic.c
index a23eb239fb17..f813ca3a5cd5 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -716,10 +716,31 @@ static __init int register_warn_debugfs(void)
/* Don't care about failure */
debugfs_create_file_unsafe("clear_warn_once", 0600, NULL,
&warn_once_reset, &clear_warn_once_fops);
+
+ /* if a bootarg was used, set the initial timer */
+ if (warn_once_reset)
+ warn_once_set(NULL, warn_once_reset);
+
return 0;
}

device_initcall(register_warn_debugfs);
+
+static int __init warn_once_setup(char *s)
+{
+ int r;
+
+ if (!s)
+ return -EINVAL;
+
+ r = kstrtoull(s, 0, &warn_once_reset);
+ if (r)
+ return r;
+
+ return 1;
+}
+__setup("warn_once_reset=", warn_once_setup);
+
#endif

#ifdef CONFIG_STACKPROTECTOR
--
2.25.1

2020-11-27 16:18:46

by Petr Mladek

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

On Thu 2020-11-26 01:30:26, Paul Gortmaker wrote:
> The existing clear_warn_once functionality is currently a manually
> issued state reset via the file /sys/kernel/debug/clear_warn_once when
> debugfs is mounted. The idea being that a developer would be running
> some tests, like LTP or similar, and want to check reproducibility
> without having to reboot.
>
> But you currently can't make use of clear_warn_once unless you've got
> debugfs enabled and mounted - which may not be desired by some people
> in some deployment situations.
>
> The functionality added here allows for periodic resets in addition to
> the one-shot reset it already had. Then we allow for a boot-time setting
> of the periodic resets so it can be used even when debugfs isn't mounted.
>
> By having a periodic reset, we also open the door for having the various
> "once" functions act as long period ratelimited messages, where a sysadmin
> can pick an hour or a day reset if they are facing an issue and are
> wondering "did this just happen once, or am I only being informed once?"

What is the primary problem that you wanted to solve, please?

Do you have an example what particular printk_once() you were
interested into?

I guess that the main problem is that
/sys/kernel/debug/clear_warn_once is available only when debugfs is
mounted. And the periodic reset is just one possible solution
that looks like a nice to have. Do I get it correctly, please?

I am not completely against the idea. But I have some concerns.

1. It allows to convert printk_once() into printk_ratelimited()
with some special semantic and interface. It opens possibilities
for creativity. It might be good and it also might create
problems that are hard to foresight now.

printk_ratelimited() is problematic, definitely, see below.


2. printk_ratelimited() is typically used when a message might get
printed too often. It prevents overloading consoles, log daemons.
Also it helps to see other messages that might get lost otherwise.

I have seen many discussions about what is the right ratelimit
for a particular message. I have to admit that it was mainly
related to console speed. The messages were lost with slow
consoles. People want to see more on fast consoles.

The periodic warn once should not have this problem because the
period would typically be long. And it would produce only
one message on each location.

The problem is that it is a global setting. It would reset
all printk_once() callers. And I see two problems here:

+ Periodic reset might cause printing related problems
in the wrong order. Messages from victims first. Messages
about the root of the problem later (from next cycle).
It might create confusion.

+ People have problems to set the right ratelimit for
a particular message. It would be even bigger problem
to set the right ratelimit for the entire system.


I do not know. Maybe I am just too paranoid today. Anyway, there
are other possibilities:

+ Move clear_warn_once from debugfs to a location that is always
available. For example, into /proc

+ Allow to change printk_once() to printk_n_times() globally. I mean
that it would print the same message only N-times instead on once.
It will print only first few occurrences, so it will not have
the problem with ordering.

Any other opinion?

Best Regards,
Petr

2020-11-27 17:46:07

by Paul Gortmaker

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

[Re: [PATCH 0/3] clear_warn_once: add timed interval resetting] On 27/11/2020 (Fri 17:13) Petr Mladek wrote:

> On Thu 2020-11-26 01:30:26, Paul Gortmaker wrote:
> > The existing clear_warn_once functionality is currently a manually
> > issued state reset via the file /sys/kernel/debug/clear_warn_once when
> > debugfs is mounted. The idea being that a developer would be running
> > some tests, like LTP or similar, and want to check reproducibility
> > without having to reboot.
> >
> > But you currently can't make use of clear_warn_once unless you've got
> > debugfs enabled and mounted - which may not be desired by some people
> > in some deployment situations.
> >
> > The functionality added here allows for periodic resets in addition to
> > the one-shot reset it already had. Then we allow for a boot-time setting
> > of the periodic resets so it can be used even when debugfs isn't mounted.
> >
> > By having a periodic reset, we also open the door for having the various
> > "once" functions act as long period ratelimited messages, where a sysadmin
> > can pick an hour or a day reset if they are facing an issue and are
> > wondering "did this just happen once, or am I only being informed once?"
>
> What is the primary problem that you wanted to solve, please?

You've captured it exactly below.

>
> Do you have an example what particular printk_once() you were
> interested into?

Well, the one I encounter (directly/indirectly) most is the one I
mentioned in mainline 3ec25826ae3 - the throttling one.

> I guess that the main problem is that
> /sys/kernel/debug/clear_warn_once is available only when debugfs is
> mounted. And the periodic reset is just one possible solution
> that looks like a nice to have. Do I get it correctly, please?

That is exactly it. I wanted the functionality of the clear but w/o the
debugfs requirement, and thinking backwards from there - came up with
the timer based solution. Other uses and/or users of the periodic reset
seemed like an added bonus. Enabling sysadmins to be able to gather
more data upon seeing an issue seems like a good thing.

> I am not completely against the idea. But I have some concerns.
>
> 1. It allows to convert printk_once() into printk_ratelimited()
> with some special semantic and interface. It opens possibilities
> for creativity. It might be good and it also might create
> problems that are hard to foresight now.

Actually that problem, if it is one, existed as soon as clear_warn_once
feature was added to the kernel years ago in v4.x kernel version:

(while [ 1 ] ; do echo 1 > clear_warn_once ; sleep 1 ; done) &

The printk_once is now converted to printk_ratelimited for one second.

I thought about it a bunch, and of course we have the fact that this
extension is an opt-in thing, and hence the default is unchanged and
most people won't even know it exists, unless they actively go looking
for it in order to collect more information.

> printk_ratelimited() is problematic, definitely, see below.

I can't argue that.

>
> 2. printk_ratelimited() is typically used when a message might get
> printed too often. It prevents overloading consoles, log daemons.
> Also it helps to see other messages that might get lost otherwise.
>
> I have seen many discussions about what is the right ratelimit
> for a particular message. I have to admit that it was mainly
> related to console speed. The messages were lost with slow
> consoles. People want to see more on fast consoles.

Yeah, I've seen those too, which is typically concerned with 10-1000
printk per second - but this isn't that discussion, and I don't want
it to be that discussion.

> The periodic warn once should not have this problem because the
> period would typically be long. And it would produce only
> one message on each location.

Correct. I even entertained setting a minimum, like 1m or 5m, but then
considered the old unix rule about the kernel not setting policy.
That said, if it made people more at ease, I'd be OK with setting a 1m
minimum on the reset - I can't think of a use case where faster than
that would ever make sense.

> The problem is that it is a global setting. It would reset
> all printk_once() callers. And I see two problems here:
>
> + Periodic reset might cause printing related problems
> in the wrong order. Messages from victims first. Messages
> about the root of the problem later (from next cycle).
> It might create confusion.

The out-of-order problem exists already just like the ratelimited
"conversion" exists already as shown above - using the same script.

That aside, the out of order problem assumes 1) you have a linked pair
printk_once("root cause") and printk_once("victim") and 2) that they are
separated in time by something on the order of minutes. Even if both #1
and #2 are true, the sysadmin will still see the very 1st "matched pair".

At that point -- it will be the sysadmin who has enabled the reset in
order to collect more data after seeing the matched pair with the
one-shot defaults, so they know what they are looking at already.

> + People have problems to set the right ratelimit for
> a particular message. It would be even bigger problem
> to set the right ratelimit for the entire system.

I can't argue with that, other than to say again that this is a
different problem space. Even though I didn't set a minimum, the
periodic implementation itself does set a minimum of two seconds.

> I do not know. Maybe I am just too paranoid today. Anyway, there
> are other possibilities:
>
> + Move clear_warn_once from debugfs to a location that is always
> available. For example, into /proc

I don't have a problem with that, other than won't we have to maintain
both interfaces forever?

> + Allow to change printk_once() to printk_n_times() globally. I mean
> that it would print the same message only N-times instead on once.
> It will print only first few occurrences, so it will not have
> the problem with ordering.

As per above, you have the ordering "problem" already with the existing
clear_warn_once implementation and one line added to /etc/rc.local

That aside, the printk_once and "N times" solution always have the issue
of a sysadmin thinking "oh I guess whatever the issue was, magically
fixed itself". I've never liked that aspect, but that is the design.

You and I might go and look at the source and see it was capped at N
times, but I think it is unrealistic to think all sysadmins would.

Good questions - good feedback. I hope my answers helped.

Paul.
--

>
> Any other opinion?
>
> Best Regards,
> Petr

2020-11-30 03:15:16

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

On Thu, Nov 26, 2020 at 01:30:26AM -0500, Paul Gortmaker wrote:
> But you currently can't make use of clear_warn_once unless you've got
> debugfs enabled and mounted - which may not be desired by some people
> in some deployment situations.

Seems awfully special purpose. The problem with debugfs is security,
or is it no convenient process that could do cron like functionality?

If it's the first, perhaps what they really need is a way to get
a partial debugfs?

-Andi

2020-11-30 06:07:30

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

On (20/11/27 17:13), Petr Mladek wrote:
>
> + Move clear_warn_once from debugfs to a location that is always
> available. For example, into /proc

Or a printk module param, which user-space can write to from crontab?
Hmm, but this has potential of becoming another /proc/sys/vm/drop_caches
though.

-ss

2020-11-30 16:29:33

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

On Fri, 27 Nov 2020 17:13:52 +0100
Petr Mladek <[email protected]> wrote:

> I do not know. Maybe I am just too paranoid today. Anyway, there
> are other possibilities:

This is another reason I think the resolution should be in minutes and not
seconds. It would be less of an issue if it could dump all warnings only
once a minute than once every two seconds.

>
> + Move clear_warn_once from debugfs to a location that is always
> available. For example, into /proc

I was thinking of /proc/sys/ as well.

-- Steve

2020-11-30 17:43:35

by Paul Gortmaker

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

[Re: [PATCH 0/3] clear_warn_once: add timed interval resetting] On 29/11/2020 (Sun 19:08) Andi Kleen wrote:

> On Thu, Nov 26, 2020 at 01:30:26AM -0500, Paul Gortmaker wrote:
> > But you currently can't make use of clear_warn_once unless you've got
> > debugfs enabled and mounted - which may not be desired by some people
> > in some deployment situations.
>
> Seems awfully special purpose. The problem with debugfs is security,
> or is it no convenient process that could do cron like functionality?

My understanding is that it is a bit of both. As users of rt tasks,
they won't be running anything like cron that could add to OS jitter on
the (presumably minimal) rootfs - so they were looking for a clean
engineered solution with near zero overhead, that they could easily
deploy on all nodes after the rt tuning was 99% completed and node
images had been bundled. Just to be sure everything was operating as
they'd aimed to achieve.

I thought a boot arg (and the internal timer) seemed like a good fit to
that requirement. No kernel or rootfs rebuilding required. And I
figured others might be in the same boat and could use it too.

Paul.
--

>
> If it's the first, perhaps what they really need is a way to get
> a partial debugfs?
>
> -Andi

2020-12-01 12:54:10

by Petr Mladek

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

On Mon 2020-11-30 12:38:43, Paul Gortmaker wrote:
> [Re: [PATCH 0/3] clear_warn_once: add timed interval resetting] On 29/11/2020 (Sun 19:08) Andi Kleen wrote:
>
> > On Thu, Nov 26, 2020 at 01:30:26AM -0500, Paul Gortmaker wrote:
> > > But you currently can't make use of clear_warn_once unless you've got
> > > debugfs enabled and mounted - which may not be desired by some people
> > > in some deployment situations.
> >
> > Seems awfully special purpose. The problem with debugfs is security,
> > or is it no convenient process that could do cron like functionality?
>
> My understanding is that it is a bit of both. As users of rt tasks,
> they won't be running anything like cron that could add to OS jitter on
> the (presumably minimal) rootfs - so they were looking for a clean
> engineered solution with near zero overhead, that they could easily
> deploy on all nodes after the rt tuning was 99% completed and node
> images had been bundled. Just to be sure everything was operating as
> they'd aimed to achieve.

Is this feature requested by RT people?
Or is it just a possible use-case?

I am not sure that RT is a really good example. The cron job is only
part of the problem. The message would create a noise on its own.
It would be shown on console or read/stored by a userspace log
daemon. I am not sure that RT people would really want to use this.

That said, I still do not have strong opinion about the feature.
It might make sense on its own. But I still see it as a workaround
for another problem.

Non-trivial periodic tasks sometimes cause problems. And we do not
know how big avalanche of messages it might restart.

Also the once is sometimes used on purpose. It prevents repeated delays
on fast paths. I wonder if it can sometimes even prevent recursion.

I know that everything is possible already now. But this patchset
makes it more visible and easier to use.

Best Regards,
Petr

2020-12-01 18:09:25

by Paul Gortmaker

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

[Re: [PATCH 0/3] clear_warn_once: add timed interval resetting] On 01/12/2020 (Tue 13:49) Petr Mladek wrote:

[...]

> Is this feature requested by RT people?
> Or is it just a possible use-case?
>
> I am not sure that RT is a really good example. The cron job is only
> part of the problem. The message would create a noise on its own.
> It would be shown on console or read/stored by a userspace log
> daemon. I am not sure that RT people would really want to use this.

To be clear, no RT person requested this, and it is just one possible
use case. Enabling the sysadmin to be able to collect more data on
recurrence equally applies to WARN_ONCE as it does printk_once.

> That said, I still do not have strong opinion about the feature.
> It might make sense on its own. But I still see it as a workaround
> for another problem.

I'm not sure how it could be a workaround for anything, really. It
doesn't hide anything -- it would instead possibly cause more output.
It enables a sysadmin to collect more data on recurrence when asked to
by a developer like one of us -- without having to ask the sysadmin to
be rebuilding the kernel or altering the rootfs. "Please boot with
this boot-arg, and run for 3 days and report what you see."

If you get a WARN_ONCE, and choose to ignore it - you have already
decided you are OK with running with something clearly broken (not
good). Being able to easily check if it happens again over time seems
like a good step towards resolving the issue vs. ignoring it.

> Non-trivial periodic tasks sometimes cause problems. And we do not
> know how big avalanche of messages it might restart.

Without specifics, I can't really address what problems you speak of.
But with a 2m minimum, if we add that - we can definitely say the risk
of "big avalanche of messages" is zero and not an issue. We could even
use 5 or 10m minimum w/o really changing what I'm trying to achieve here.

> Also the once is sometimes used on purpose. It prevents repeated delays
> on fast paths. I wonder if it can sometimes even prevent recursion.

Again, I can't really address an open speculation like that, other than
to say if we do have an example of such recursion blocking, we should
code it explicitly, so it doesn't hide as a trap and blow up if someone
removes the "_once" at a later date as a part of a mainline change.

> I know that everything is possible already now. But this patchset
> makes it more visible and easier to use.

So, I have one last idea that may address your concern of people abusing
the reset variable like it is something to be used everyday, blindly.

What if we unconditionally set TAINT_USER once it is used? That also
assists with the fact that such abuse is possible now even without
any of these changes applied, as you have acknowledged.

We'd be making it 100% clear that a person shouldn't be hammering away
on the reset simply because it happens to be there. The taint would
make it clear it isn't a "feature" but instead a debugging/information
gathering aid to only be used on occasion with a specific goal in mind.

I could do a v2 with a TAINT_USER addition, and a conversion to minutes,
with a 5m minimum. But I won't spam people with that unless it resolves
the concerns that you (and anyone else) might have with misuse.

If people don't see the value in it easing data collection once an issue
is spotted, I'm fine with that and will shelf the patch set, and thank
people for their valuable time and feedback.

Paul.
--

>
> Best Regards,
> Petr

2020-12-01 22:35:27

by Petr Mladek

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

On Fri 2020-11-27 12:43:16, Paul Gortmaker wrote:
> > On Thu 2020-11-26 01:30:26, Paul Gortmaker wrote:
> > + Move clear_warn_once from debugfs to a location that is always
> > available. For example, into /proc
>
> I don't have a problem with that, other than won't we have to maintain
> both interfaces forever?

Yes, we would. But this patchset adds a new interface as well.
In addition, it adds also new functionality that might create new
scenarios.

Again, I am not strongly against it. But I have to think more
about it.

Best Regards,
Petr

PS: I did not comment other parts of this mail. They were either
discussed elsewhere in this thread. Or I did not have
anything to add.

2020-12-09 16:43:47

by Petr Mladek

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

On Thu 2020-11-26 01:30:26, Paul Gortmaker wrote:
> The existing clear_warn_once functionality is currently a manually
> issued state reset via the file /sys/kernel/debug/clear_warn_once when
> debugfs is mounted. The idea being that a developer would be running
> some tests, like LTP or similar, and want to check reproducibility
> without having to reboot.
>
> But you currently can't make use of clear_warn_once unless you've got
> debugfs enabled and mounted - which may not be desired by some people
> in some deployment situations.
>
> The functionality added here allows for periodic resets in addition to
> the one-shot reset it already had. Then we allow for a boot-time setting
> of the periodic resets so it can be used even when debugfs isn't mounted.
>
> By having a periodic reset, we also open the door for having the various
> "once" functions act as long period ratelimited messages, where a sysadmin
> can pick an hour or a day reset if they are facing an issue and are
> wondering "did this just happen once, or am I only being informed once?"

OK, I though more about it and I NACK this patchset.

My reason:

1. The primary purpose was to provide a way to reset warn_once() without
debugfs. From this POV, the solution is rather complicated: timers
and another kernel parameter.

2. I am not aware of any convincing argument why debugfs could not be
mounted on the debugged system.

3. Debugfs provides many more debugging facilities. It is designed for
this purpose. It does not look like a good strategy to provide
alternative interfaces just to avoid it.

4. There were mentioned several other use cases for this feature,
like RT systems. But it was not clear that it was really needed
or that people would really use it.

5. Some code might even rely on that it is called only once, see commit
dfbf2897d00499f94cd ("bug: set warn variable before calling
WARN()") or the recent
https://lore.kernel.org/r/[email protected]

It should better stay as debugging feature that should be used with
care.


6. It creates system wide ratelimited printk().

We have printk_ratelimited() for this. And it is quite problematic.
It is supposed to prevent flood of printk() messages. But it does
not work well because the limits depend on more factors, like:
system size, conditions, console speed.

Yes, the proposed feature is supposed to solve another problem
(lack of messages). But it is a global action that would
re-enable >1000 messages that were limited to be printed
only once because they could be too frequent. As a result:

+ it might cause flood of printk() messages

+ it is hard to define a good system wide time limit;
it was even unclear what should be the lower limit.

+ it will restart the messages at some "random" point,
so that the relation of the reported events would
be unclear.

From the API point of view:

+ printk_ratelimited() is used when we want to see that a
problem is still there. It is per-message setting.

+ printk_once() is used when even printk_ratelimited() would
be too much. It is per-message setting.

+ The new printk_repeated_once() is a strange mix of this two
with the global setting. It does not fit much.


Best Regards,
Petr

PS: I did not answer your last mail because it looked like an endless
fight over words or point of views. I decided to make a summary
of my view instead. These are reason why I nacked it.

I know that there might be different views but so far no arguments
changed mine. And I do not know how to explain it better.

2020-12-09 17:25:33

by Paul Gortmaker

[permalink] [raw]
Subject: Re: [PATCH 0/3] clear_warn_once: add timed interval resetting

[Re: [PATCH 0/3] clear_warn_once: add timed interval resetting] On 09/12/2020 (Wed 17:37) Petr Mladek wrote:

> On Thu 2020-11-26 01:30:26, Paul Gortmaker wrote:
> > The existing clear_warn_once functionality is currently a manually
> > issued state reset via the file /sys/kernel/debug/clear_warn_once when
> > debugfs is mounted. The idea being that a developer would be running
> > some tests, like LTP or similar, and want to check reproducibility
> > without having to reboot.
> >
> > But you currently can't make use of clear_warn_once unless you've got
> > debugfs enabled and mounted - which may not be desired by some people
> > in some deployment situations.
> >
> > The functionality added here allows for periodic resets in addition to
> > the one-shot reset it already had. Then we allow for a boot-time setting
> > of the periodic resets so it can be used even when debugfs isn't mounted.
> >
> > By having a periodic reset, we also open the door for having the various
> > "once" functions act as long period ratelimited messages, where a sysadmin
> > can pick an hour or a day reset if they are facing an issue and are
> > wondering "did this just happen once, or am I only being informed once?"
>
> OK, I though more about it and I NACK this patchset.

Not a problem. Thanks again for your time and explaining your thoughts.

At least it is out there if anyone wants to use it and they can follow
the discussion here when considering the pros/cons of doing so.

Paul.
--

>
> My reason:
>
> 1. The primary purpose was to provide a way to reset warn_once() without
> debugfs. From this POV, the solution is rather complicated: timers
> and another kernel parameter.
>
> 2. I am not aware of any convincing argument why debugfs could not be
> mounted on the debugged system.
>
> 3. Debugfs provides many more debugging facilities. It is designed for
> this purpose. It does not look like a good strategy to provide
> alternative interfaces just to avoid it.
>
> 4. There were mentioned several other use cases for this feature,
> like RT systems. But it was not clear that it was really needed
> or that people would really use it.
>
> 5. Some code might even rely on that it is called only once, see commit
> dfbf2897d00499f94cd ("bug: set warn variable before calling
> WARN()") or the recent
> https://lore.kernel.org/r/[email protected]
>
> It should better stay as debugging feature that should be used with
> care.
>
>
> 6. It creates system wide ratelimited printk().
>
> We have printk_ratelimited() for this. And it is quite problematic.
> It is supposed to prevent flood of printk() messages. But it does
> not work well because the limits depend on more factors, like:
> system size, conditions, console speed.
>
> Yes, the proposed feature is supposed to solve another problem
> (lack of messages). But it is a global action that would
> re-enable >1000 messages that were limited to be printed
> only once because they could be too frequent. As a result:
>
> + it might cause flood of printk() messages
>
> + it is hard to define a good system wide time limit;
> it was even unclear what should be the lower limit.
>
> + it will restart the messages at some "random" point,
> so that the relation of the reported events would
> be unclear.
>
> From the API point of view:
>
> + printk_ratelimited() is used when we want to see that a
> problem is still there. It is per-message setting.
>
> + printk_once() is used when even printk_ratelimited() would
> be too much. It is per-message setting.
>
> + The new printk_repeated_once() is a strange mix of this two
> with the global setting. It does not fit much.
>
>
> Best Regards,
> Petr
>
> PS: I did not answer your last mail because it looked like an endless
> fight over words or point of views. I decided to make a summary
> of my view instead. These are reason why I nacked it.
>
> I know that there might be different views but so far no arguments
> changed mine. And I do not know how to explain it better.