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/20201029142406.3c468...@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
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.3c468...@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
[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
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
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
[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
On Fri, 27 Nov 2020 17:13:52 +0100 Petr Mladek 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
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
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
[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(&quo
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
[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 Cc: Petr Mladek Cc: Sergey Senozhatsky Cc: Steven Rostedt Cc: John Ogness 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
