| Message ID | 20220819221731.480795-7-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | The panic notifiers refactor - fixes/clean-ups (V3) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
On 19/08/2022 19:17, Guilherme G. Piccoli wrote: > Currently the tracing dump_on_oops feature is implemented through > separate notifiers, one for die/oops and the other for panic; > given they have the same functionality, let's unify them. > > Also improve the function comment and change the priority of the > notifier to make it execute earlier, avoiding showing useless trace > data (like the callback names for the other notifiers); finally, > we also removed an unnecessary header inclusion. > > Cc: Petr Mladek <pmladek@suse.com> > Cc: Sergei Shtylyov <sergei.shtylyov@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > > --- > > V3: > - Removed goto usage, as per Steven suggestion (thanks!). > > V2: > - Different approach; instead of using IDs to distinguish die and > panic events, rely on address comparison like other notifiers do > and as per Petr's suggestion; > > - Removed ACK from Steven since the code changed. > > [...] Hi Steve, Alan - sorry for the ping (and I'm aware you're OOO Steve, saw your auto-response email heh). So, is this version good enough? Appreciate the reviews and in case it's good, let me know your preference for picking it in your tree - I could resend the patch alone if you prefer (not in the series), for example. Thanks, Guilherme
On 19/08/2022 19:17, Guilherme G. Piccoli wrote: > Currently the tracing dump_on_oops feature is implemented through > separate notifiers, one for die/oops and the other for panic; > given they have the same functionality, let's unify them. > > Also improve the function comment and change the priority of the > notifier to make it execute earlier, avoiding showing useless trace > data (like the callback names for the other notifiers); finally, > we also removed an unnecessary header inclusion. > > Cc: Petr Mladek <pmladek@suse.com> > Cc: Sergei Shtylyov <sergei.shtylyov@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > > --- > > V3: > - Removed goto usage, as per Steven suggestion (thanks!). > > V2: > - Different approach; instead of using IDs to distinguish die and > panic events, rely on address comparison like other notifiers do > and as per Petr's suggestion; > > - Removed ACK from Steven since the code changed. > Hi Steven, apologies for the re-ping. Is there anything else to do in this one, or do you think it's good enough? Thanks, Guilherme
On Fri, 19 Aug 2022 19:17:26 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote: > Currently the tracing dump_on_oops feature is implemented through > separate notifiers, one for die/oops and the other for panic; > given they have the same functionality, let's unify them. > > Also improve the function comment and change the priority of the > notifier to make it execute earlier, avoiding showing useless trace > data (like the callback names for the other notifiers); finally, > we also removed an unnecessary header inclusion. > > Cc: Petr Mladek <pmladek@suse.com> > Cc: Sergei Shtylyov <sergei.shtylyov@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> Sorry for the late reply. Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> -- Steve
On 20/10/2022 18:29, Steven Rostedt wrote: > [...] > Sorry for the late reply. > > Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > -- Steve No need for apologies! Appreciate your review/ack =) Could you pick it in your tree? Or do you prefer that I re-send as a solo patch, with your ACK? Cheers, Guilherme
On Thu, 20 Oct 2022 18:53:43 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote: > Could you pick it in your tree? Or do you prefer that I re-send as a > solo patch, with your ACK? I wasn't sure there were any dependencies on this. If not, I can take it. -- Steve
On 20/10/2022 19:22, Steven Rostedt wrote: > On Thu, 20 Oct 2022 18:53:43 -0300 > "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote: > >> Could you pick it in your tree? Or do you prefer that I re-send as a >> solo patch, with your ACK? > > I wasn't sure there were any dependencies on this. If not, I can take it. > > -- Steve Thank you! No dependency at all...I just piled a bunch of independent panic notifiers fixes, so they can be reviewed by the maintainers and picked individually. Some maintainers though prefer them to be sent individually, as solo patches...hence my question. Cheers, Guilherme
On 20/10/2022 19:22, Steven Rostedt wrote: > On Thu, 20 Oct 2022 18:53:43 -0300 > "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote: > >> Could you pick it in your tree? Or do you prefer that I re-send as a >> solo patch, with your ACK? > > I wasn't sure there were any dependencies on this. If not, I can take it. > > -- Steve Hi Steve, I'm really sorry for the re-ping. Just wanted to know what's the status here, not sure if you picked this one (checked linux-trace/for-next, didn't see it there) or planning to. Thanks in advance, Guilherme P.S. Trimmed huge CC list...
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index d3005279165d..0bacdbcb132f 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -19,7 +19,6 @@ #include <linux/kallsyms.h> #include <linux/security.h> #include <linux/seq_file.h> -#include <linux/notifier.h> #include <linux/irqflags.h> #include <linux/debugfs.h> #include <linux/tracefs.h> @@ -9776,40 +9775,40 @@ static __init int tracer_init_tracefs(void) fs_initcall(tracer_init_tracefs); -static int trace_panic_handler(struct notifier_block *this, - unsigned long event, void *unused) -{ - if (ftrace_dump_on_oops) - ftrace_dump(ftrace_dump_on_oops); - return NOTIFY_OK; -} +static int trace_die_panic_handler(struct notifier_block *self, + unsigned long ev, void *unused); static struct notifier_block trace_panic_notifier = { - .notifier_call = trace_panic_handler, - .next = NULL, - .priority = 150 /* priority: INT_MAX >= x >= 0 */ + .notifier_call = trace_die_panic_handler, + .priority = INT_MAX - 1, }; -static int trace_die_handler(struct notifier_block *self, - unsigned long val, - void *data) -{ - switch (val) { - case DIE_OOPS: - if (ftrace_dump_on_oops) - ftrace_dump(ftrace_dump_on_oops); - break; - default: - break; - } - return NOTIFY_OK; -} - static struct notifier_block trace_die_notifier = { - .notifier_call = trace_die_handler, - .priority = 200 + .notifier_call = trace_die_panic_handler, + .priority = INT_MAX - 1, }; +/* + * The idea is to execute the following die/panic callback early, in order + * to avoid showing irrelevant information in the trace (like other panic + * notifier functions); we are the 2nd to run, after hung_task/rcu_stall + * warnings get disabled (to prevent potential log flooding). + */ +static int trace_die_panic_handler(struct notifier_block *self, + unsigned long ev, void *unused) +{ + if (!ftrace_dump_on_oops) + return NOTIFY_DONE; + + /* The die notifier requires DIE_OOPS to trigger */ + if (self == &trace_die_notifier && ev != DIE_OOPS) + return NOTIFY_DONE; + + ftrace_dump(ftrace_dump_on_oops); + + return NOTIFY_DONE; +} + /* * printk is set to max of 1024, we really don't need it that big. * Nothing should be printing 1000 characters anyway.
Currently the tracing dump_on_oops feature is implemented through separate notifiers, one for die/oops and the other for panic; given they have the same functionality, let's unify them. Also improve the function comment and change the priority of the notifier to make it execute earlier, avoiding showing useless trace data (like the callback names for the other notifiers); finally, we also removed an unnecessary header inclusion. Cc: Petr Mladek <pmladek@suse.com> Cc: Sergei Shtylyov <sergei.shtylyov@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- V3: - Removed goto usage, as per Steven suggestion (thanks!). V2: - Different approach; instead of using IDs to distinguish die and panic events, rely on address comparison like other notifiers do and as per Petr's suggestion; - Removed ACK from Steven since the code changed. kernel/trace/trace.c | 55 ++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 28 deletions(-)