| Message ID | 20220719195325.402745-9-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | The panic notifiers refactor strikes back - fixes/clean-ups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
On 07/19/22 at 04:53pm, 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> > > --- > > 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(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index b8dd54627075..2a436b645c70 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> > @@ -9777,40 +9776,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) > + goto out; > + > + if (self == &trace_die_notifier && ev != DIE_OOPS) > + goto out; Although the switch-case code of original trace_die_handler() is werid, this unification is not much more comfortable. Just personal feeling from code style, not strong opinion. Leave it to trace reviewers. > + > + ftrace_dump(ftrace_dump_on_oops); > + > +out: > + 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. > -- > 2.37.1 >
On 08/03/22 at 05:36pm, Baoquan He wrote: > On 07/19/22 at 04:53pm, 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> > > > > --- > > > > 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(-) > > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index b8dd54627075..2a436b645c70 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> > > @@ -9777,40 +9776,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) > > + goto out; > > + > > + if (self == &trace_die_notifier && ev != DIE_OOPS) > > + goto out; > > Although the switch-case code of original trace_die_handler() is werid, > this unification is not much more comfortable. Just personal feeling > from code style, not strong opinion. Leave it to trace reviewers. Please ignore this comment. I use b4 to grab this patchset and applied, and started to check patch one by one. Then I realize it's all about cleanups which have got consensus in earlier rounds. Hope it can be merged when other people's concern is addressed, the whole series looks good to me, I have no strong concern to them. > > > + > > + ftrace_dump(ftrace_dump_on_oops); > > + > > +out: > > + 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. > > -- > > 2.37.1 > > >
On 03/08/2022 06:52, Baoquan He wrote: > [...] >> >> Although the switch-case code of original trace_die_handler() is werid, >> this unification is not much more comfortable. Just personal feeling >> from code style, not strong opinion. Leave it to trace reviewers. > > Please ignore this comment. > > I use b4 to grab this patchset and applied, and started to check patch > one by one. Then I realize it's all about cleanups which have got > consensus in earlier rounds. Hope it can be merged when other people's > concern is addressed, the whole series looks good to me, I have no > strong concern to them. > Thanks a lot for your reviews Baoquan, much appreciated =)
On 19/07/2022 16:53, 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> > > --- > > 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(-) > [...] Hi Sergei / Steve, do you think this version is good now, after your last round of reviews? Thanks, Guilherme
On Tue, 19 Jul 2022 16:53:21 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote: Sorry for the late review, but this fell to the bottom of my queue :-/ > +/* > + * 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) > + goto out; > + > + if (self == &trace_die_notifier && ev != DIE_OOPS) > + goto out; I really hate gotos that are not for clean ups. > + > + ftrace_dump(ftrace_dump_on_oops); > + > +out: > + return NOTIFY_DONE; > +} > + Just do: 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; } Thanks, Other than that, Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> -- Steve
On Tue, Aug 16, 2022 at 10:14:45AM -0400, Steven Rostedt wrote: > On Tue, 19 Jul 2022 16:53:21 -0300 > "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote: > > > Sorry for the late review, but this fell to the bottom of my queue :-/ > > > +/* > > + * 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) > > + goto out; > > + > > + if (self == &trace_die_notifier && ev != DIE_OOPS) > > + goto out; > > I really hate gotos that are not for clean ups. > > > + > > + ftrace_dump(ftrace_dump_on_oops); > > + > > +out: > > + return NOTIFY_DONE; > > +} > > + > > Just do: > > 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; > } Or better yet: if (ftrace_dump_on_oops) { /* The die notifier requires DIE_OOPS to trigger */ if (self != &trace_die_notifier || ev == DIE_OOPS) ftrace_dump(ftrace_dump_on_oops); } return NOTIFY_DONE; Alan Stern > Thanks, > > Other than that, Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > -- Steve
On Tue, 16 Aug 2022 10:57:20 -0400 Alan Stern <stern@rowland.harvard.edu> wrote: > > 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; > > } > > Or better yet: > > if (ftrace_dump_on_oops) { > > /* The die notifier requires DIE_OOPS to trigger */ > if (self != &trace_die_notifier || ev == DIE_OOPS) > ftrace_dump(ftrace_dump_on_oops); > } > return NOTIFY_DONE; > That may be more consolidated but less easy to read and follow. This is far from a fast path. As I maintain this bike-shed, I prefer the one I suggested ;-) -- Steve
On 16/08/2022 12:52, Steven Rostedt wrote: > On Tue, 16 Aug 2022 10:57:20 -0400 > Alan Stern <stern@rowland.harvard.edu> wrote: > >>> 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; >>> } >> >> Or better yet: >> >> if (ftrace_dump_on_oops) { >> >> /* The die notifier requires DIE_OOPS to trigger */ >> if (self != &trace_die_notifier || ev == DIE_OOPS) >> ftrace_dump(ftrace_dump_on_oops); >> } >> return NOTIFY_DONE; >> > > That may be more consolidated but less easy to read and follow. This is far > from a fast path. > > As I maintain this bike-shed, I prefer the one I suggested ;-) > > -- Steve Perfect Steve and Alan, appreciate your suggestions! I'll submit V3 using your change Steve - honestly, I'm not sure why in the heck I put a goto there, yours is basically the same code, modulo the goto heheh A braino from me, for sure! Cheers, Guilherme
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index b8dd54627075..2a436b645c70 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> @@ -9777,40 +9776,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) + goto out; + + if (self == &trace_die_notifier && ev != DIE_OOPS) + goto out; + + ftrace_dump(ftrace_dump_on_oops); + +out: + 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> --- 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(-)