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

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

[Paul Gortmaker]

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 <ak@linux.intel.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>

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

[PATCH 1/3] clear_warn_once: expand debugfs to include read support

[Paul Gortmaker]

The existing clear_warn_once variable is write-only; used as per the
documentation to reset the warn_once to as-booted state with:

	echo 1 > /sys/kernel/debug/clear_warn_once

The objective is to expand on that functionality, which requires the
debugfs variable to be read/write and not just write-only.

Here, we deal with the debugfs boilerplate associated with converting
it from write-only to read-write, in order to factor that out for easier
review, and for what may be a possible future useful bisect point.

Existing functionality is unchanged - the only difference is that we
have tied in a variable that lets you now read the variable and see
the last value written.

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 kernel/panic.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 332736a72a58..1d425970a50c 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -654,21 +654,36 @@ EXPORT_SYMBOL(__warn_printk);
 
 /* Support resetting WARN*_ONCE state */
 
-static int clear_warn_once_set(void *data, u64 val)
+static u64 warn_once_reset;
+
+static void do_clear_warn_once(void)
 {
 	generic_bug_clear_once();
 	memset(__start_once, 0, __end_once - __start_once);
+}
+
+static int warn_once_get(void *data, u64 *val)
+{
+	*val = warn_once_reset;
+	return 0;
+}
+
+static int warn_once_set(void *data, u64 val)
+{
+	warn_once_reset = val;
+
+	do_clear_warn_once();
 	return 0;
 }
 
-DEFINE_DEBUGFS_ATTRIBUTE(clear_warn_once_fops, NULL, clear_warn_once_set,
-			 "%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(clear_warn_once_fops, warn_once_get, warn_once_set,
+			 "%llu\n");
 
 static __init int register_warn_debugfs(void)
 {
 	/* Don't care about failure */
-	debugfs_create_file_unsafe("clear_warn_once", 0200, NULL, NULL,
-				   &clear_warn_once_fops);
+	debugfs_create_file_unsafe("clear_warn_once", 0600, NULL,
+				   &warn_once_reset, &clear_warn_once_fops);
 	return 0;
 }
 
-- 
2.25.1

[PATCH 2/3] clear_warn_once: bind a timer to written reset value

[Paul Gortmaker]

Existing documentation has a write of "1" to clear/reset all the
WARN_ONCE and similar to the as-booted state, so they can possibly
be re-triggered again during debugging/testing.

But having them auto-reset once a day, or once a week, might shed
valuable information to a sysadmin on what the system is doing.

Here we extend the existing debugfs variable to bind a timer to the
written value N, so that it will reset every N seconds, for N>1.
Writing a zero will clear any previously set timer value.

The pre-existing behaviour of writing N=1 will do a one-shot clear.

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 .../admin-guide/clearing-warn-once.rst        |  9 ++++++
 kernel/panic.c                                | 32 +++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/Documentation/admin-guide/clearing-warn-once.rst b/Documentation/admin-guide/clearing-warn-once.rst
index 211fd926cf00..93cf3ba0b57d 100644
--- a/Documentation/admin-guide/clearing-warn-once.rst
+++ b/Documentation/admin-guide/clearing-warn-once.rst
@@ -7,3 +7,12 @@ echo 1 > /sys/kernel/debug/clear_warn_once
 
 clears the state and allows the warnings to print once again.
 This can be useful after test suite runs to reproduce problems.
+
+Values greater than one set a timer for a periodic state reset; e.g.
+
+echo 3600 > /sys/kernel/debug/clear_warn_once
+
+will establish an hourly state reset, effectively turning WARN_ONCE
+into a long period rate-limited warning.
+
+Writing a value of zero (or one) will remove any previously set timer.
diff --git a/kernel/panic.c b/kernel/panic.c
index 1d425970a50c..a23eb239fb17 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -655,6 +655,7 @@ EXPORT_SYMBOL(__warn_printk);
 /* Support resetting WARN*_ONCE state */
 
 static u64 warn_once_reset;
+static bool warn_timer_active;
 
 static void do_clear_warn_once(void)
 {
@@ -662,6 +663,14 @@ static void do_clear_warn_once(void)
 	memset(__start_once, 0, __end_once - __start_once);
 }
 
+static void timer_warn_once(struct timer_list *timer)
+{
+	do_clear_warn_once();
+	timer->expires = jiffies + warn_once_reset * HZ;
+	add_timer(timer);
+}
+static DEFINE_TIMER(warn_reset_timer, timer_warn_once);
+
 static int warn_once_get(void *data, u64 *val)
 {
 	*val = warn_once_reset;
@@ -672,6 +681,29 @@ static int warn_once_set(void *data, u64 val)
 {
 	warn_once_reset = val;
 
+	if (val > 1) {		/* set/reset new timer */
+		unsigned long expires = jiffies + val * HZ;
+
+		if (warn_timer_active) {
+			mod_timer(&warn_reset_timer, expires);
+		} else {
+			warn_timer_active = 1;
+			warn_reset_timer.expires = expires;
+			add_timer(&warn_reset_timer);
+		}
+		return 0;
+	}
+
+	if (warn_timer_active) {
+		del_timer_sync(&warn_reset_timer);
+		warn_timer_active = 0;
+	}
+	warn_once_reset = 0;
+
+	if (val == 0)		/* cleared timer, we are done */
+		return 0;
+
+	/* Getting here means val == 1  --->  so clear existing data */
 	do_clear_warn_once();
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 2/3] clear_warn_once: bind a timer to written reset value

[Steven Rostedt]

On Thu, 26 Nov 2020 01:30:28 -0500
Paul Gortmaker <paul.gortmaker@windriver.com> wrote:

> +++ b/Documentation/admin-guide/clearing-warn-once.rst
> @@ -7,3 +7,12 @@ echo 1 > /sys/kernel/debug/clear_warn_once
>  
>  clears the state and allows the warnings to print once again.
>  This can be useful after test suite runs to reproduce problems.
> +
> +Values greater than one set a timer for a periodic state reset; e.g.
> +
> +echo 3600 > /sys/kernel/debug/clear_warn_once

I wonder if the value should be in minutes and not seconds, otherwise, a
wrong value could possibly DoS the machine, if you were to write 2 into it.
If there were a lot of warnings in high frequency events.

Or is dumping out a bunch of warnings every 2 seconds not be a problem?

Anyway, would there ever be a need to have it cleared in less than 1 minute
intervals?

-- Steve


> +
> +will establish an hourly state reset, effectively turning WARN_ONCE
> +into a long period rate-limited warning.

Re: [PATCH 2/3] clear_warn_once: bind a timer to written reset value

[Paul Gortmaker]

[Re: [PATCH 2/3] clear_warn_once: bind a timer to written reset value] On 30/11/2020 (Mon 11:20) Steven Rostedt wrote:

> On Thu, 26 Nov 2020 01:30:28 -0500
> Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> 
> > +++ b/Documentation/admin-guide/clearing-warn-once.rst
> > @@ -7,3 +7,12 @@ echo 1 > /sys/kernel/debug/clear_warn_once
> >  
> >  clears the state and allows the warnings to print once again.
> >  This can be useful after test suite runs to reproduce problems.
> > +
> > +Values greater than one set a timer for a periodic state reset; e.g.
> > +
> > +echo 3600 > /sys/kernel/debug/clear_warn_once
> 
> I wonder if the value should be in minutes and not seconds, otherwise, a
> wrong value could possibly DoS the machine, if you were to write 2 into it.
> If there were a lot of warnings in high frequency events.
> 
> Or is dumping out a bunch of warnings every 2 seconds not be a problem?

It doesn't seem to be a problem - at least in that running a defconfig
build on an otherwise out of the box common distro doesn't seem to trip
any WARN or printk_once events in my testing.  Of course there may be a
use case out there that is doing lots of them, however.

> Anyway, would there ever be a need to have it cleared in less than 1 minute
> intervals?

I don't think so - as I said in another follow up from last week:

https://lore.kernel.org/lkml/20201127174316.GA11748@windriver.com/

I'd also indicated in the above that I'd be fine with adding a minimum
of 1m if people feel better about that.  Also maybe moving the units to
minutes instead of seconds helps implicitly convey the intended use
better -- i.e. "don't be smashing on this every second" -- maybe that
was your point as well - and I'd agree with that.

Paul.
--

> 
> -- Steve
> 
> 
> > +
> > +will establish an hourly state reset, effectively turning WARN_ONCE
> > +into a long period rate-limited warning.

Re: [PATCH 2/3] clear_warn_once: bind a timer to written reset value

[Steven Rostedt]

On Mon, 30 Nov 2020 12:17:59 -0500
Paul Gortmaker <paul.gortmaker@windriver.com> wrote:

> > Anyway, would there ever be a need to have it cleared in less than 1 minute
> > intervals?  
> 
> I don't think so - as I said in another follow up from last week:
> 
> https://lore.kernel.org/lkml/20201127174316.GA11748@windriver.com/
> 
> I'd also indicated in the above that I'd be fine with adding a minimum
> of 1m if people feel better about that.  Also maybe moving the units to
> minutes instead of seconds helps implicitly convey the intended use
> better -- i.e. "don't be smashing on this every second" -- maybe that
> was your point as well - and I'd agree with that.

That was my second point. That is, why would anyone care about a
resolution in seconds for this?

-- Steve

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

[Paul Gortmaker]

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 <ak@linux.intel.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 .../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

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

[Petr Mladek]

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

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

[Paul Gortmaker]

[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

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

[Petr Mladek]

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.

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

[Sergey Senozhatsky]

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

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

[Steven Rostedt]

On Fri, 27 Nov 2020 17:13:52 +0100
Petr Mladek <pmladek@suse.com> 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

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

[Andi Kleen]

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

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

[Paul Gortmaker]

[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

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

[Petr Mladek]

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

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

[Paul Gortmaker]

[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

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

[Petr Mladek]

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/20201029142406.3c46855a@gandalf.local.home

   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.

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

[Paul Gortmaker]

[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/20201029142406.3c46855a@gandalf.local.home
> 
>    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.