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@xxxxxxxx> > > Cc: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxx> > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx> > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@xxxxxxxxxx> > > > > --- > > > > 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 > > > _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec