Message-ID: <20201127174316.GA11748@windriver.com>
Date:   Fri, 27 Nov 2020 12:43:16 -0500
From:   Paul Gortmaker <paul.gortmaker@...driver.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        John Ogness <john.ogness@...utronix.de>
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

Powered by blists - more mailing lists