| Message ID | 20220427224924.592546-18-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | The panic notifiers refactor | expand |
Hello! On 4/28/22 1:49 AM, 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. With the addition of panic notifier "id", this > patch makes use of such "id" to unify both functions. > > It also comments the function and changes the priority of the > notifier blocks, in order they run early compared to other > notifiers, to prevent useless trace data (like the callback > names for the other notifiers). Finally, we also removed an > unnecessary header inclusion. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > --- > kernel/trace/trace.c | 57 +++++++++++++++++++++++++------------------- > 1 file changed, 32 insertions(+), 25 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index f4de111fa18f..c1d8a3622ccc 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c [...] > @@ -9767,38 +9766,46 @@ 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) > +/* > + * 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) > + int do_dump; bool? > + > + if (!ftrace_dump_on_oops) > + return NOTIFY_DONE; > + > + switch (ev) { > + case DIE_OOPS: > + do_dump = 1; > + break; > + case PANIC_NOTIFIER: > + do_dump = 1; > + break; Why not: case DIE_OOPS: case PANIC_NOTIFIER: do_dump = 1; break; > + default: > + do_dump = 0; > + break; > + } > + > + if (do_dump) > ftrace_dump(ftrace_dump_on_oops); > - return NOTIFY_OK; > + > + return NOTIFY_DONE; > } [...] MBR, Sergey
On Fri, 29 Apr 2022 12:22:44 +0300 Sergei Shtylyov <sergei.shtylyov@gmail.com> wrote: > > + switch (ev) { > > + case DIE_OOPS: > > + do_dump = 1; > > + break; > > + case PANIC_NOTIFIER: > > + do_dump = 1; > > + break; > > Why not: > > case DIE_OOPS: > case PANIC_NOTIFIER: > do_dump = 1; > break; Agreed. Other than that. Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> -- Steve
On 29/04/2022 10:23, Steven Rostedt wrote: > On Fri, 29 Apr 2022 12:22:44 +0300 > Sergei Shtylyov <sergei.shtylyov@gmail.com> wrote: > >>> + switch (ev) { >>> + case DIE_OOPS: >>> + do_dump = 1; >>> + break; >>> + case PANIC_NOTIFIER: >>> + do_dump = 1; >>> + break; >> >> Why not: >> >> case DIE_OOPS: >> case PANIC_NOTIFIER: >> do_dump = 1; >> break; > > Agreed. > > Other than that. > > Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > -- Steve Thanks Sergei and Steven, good idea! I thought about the switch change you propose, but I confess I got a bit confused by the "fallthrough" keyword - do I need to use it? About the s/int/bool, for sure! Not sure why I didn't use bool at first...heheh Cheers, Guilherme
On Fri, 29 Apr 2022 10:46:35 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote: > Thanks Sergei and Steven, good idea! I thought about the switch change > you propose, but I confess I got a bit confused by the "fallthrough" > keyword - do I need to use it? No. The fallthrough keyword is only needed when there's code between case labels. As it is very common to list multiple cases for the same code path. That is: case DIE_OOPS: case PANIC_NOTIFIER: do_dump = 1; break; Does not need a fall through label, as there's no code between the DIE_OOPS and the PANIC_NOTIFIER. But if you had: case DIE_OOPS: x = true; case PANIC_NOTIFIER: do_dump = 1; break; Then you do. -- Steve
On 29/04/2022 10:56, Steven Rostedt wrote: > [...] > No. The fallthrough keyword is only needed when there's code between case > labels. As it is very common to list multiple cases for the same code path. > That is: > > case DIE_OOPS: > case PANIC_NOTIFIER: > do_dump = 1; > break; > > Does not need a fall through label, as there's no code between the DIE_OOPS > and the PANIC_NOTIFIER. But if you had: > > case DIE_OOPS: > x = true; > case PANIC_NOTIFIER: > do_dump = 1; > break; > > Then you do. > > -- Steve Thanks a bunch for the clarification, changed that for V2 =)
On Wed 2022-04-27 19:49:11, 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. With the addition of panic notifier "id", this > patch makes use of such "id" to unify both functions. > > It also comments the function and changes the priority of the > notifier blocks, in order they run early compared to other > notifiers, to prevent useless trace data (like the callback > names for the other notifiers). Finally, we also removed an > unnecessary header inclusion. > > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -9767,38 +9766,46 @@ 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) > +/* > + * 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) > + int do_dump; > + > + if (!ftrace_dump_on_oops) > + return NOTIFY_DONE; > + > + switch (ev) { > + case DIE_OOPS: > + do_dump = 1; > + break; > + case PANIC_NOTIFIER: > + do_dump = 1; > + break; DIE_OOPS and PANIC_NOTIFIER are from different enum. It feels like comparing apples with oranges here. IMHO, the proper way to unify the two notifiers is a check of the @self parameter. Something like: static int trace_die_panic_handler(struct notifier_block *self, unsigned long ev, void *unused) { if (self == trace_die_notifier && val != DIE_OOPS) goto out; ftrace_dump(ftrace_dump_on_oops); out: return NOTIFY_DONE; } Best Regards, Petr
On 11/05/2022 08:45, Petr Mladek wrote: > [...] > DIE_OOPS and PANIC_NOTIFIER are from different enum. > It feels like comparing apples with oranges here. > > IMHO, the proper way to unify the two notifiers is > a check of the @self parameter. Something like: > > static int trace_die_panic_handler(struct notifier_block *self, > unsigned long ev, void *unused) > { > if (self == trace_die_notifier && val != DIE_OOPS) > goto out; > > ftrace_dump(ftrace_dump_on_oops); > out: > return NOTIFY_DONE; > } > > Best Regards, > Petr OK Petr, thanks - will implement your suggestion in V2 (CC Steven) Cheers!
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index f4de111fa18f..c1d8a3622ccc 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> @@ -9767,38 +9766,46 @@ 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) +/* + * 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) + int do_dump; + + if (!ftrace_dump_on_oops) + return NOTIFY_DONE; + + switch (ev) { + case DIE_OOPS: + do_dump = 1; + break; + case PANIC_NOTIFIER: + do_dump = 1; + break; + default: + do_dump = 0; + break; + } + + if (do_dump) ftrace_dump(ftrace_dump_on_oops); - return NOTIFY_OK; + + return NOTIFY_DONE; } 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, }; /*
Currently the tracing dump_on_oops feature is implemented through separate notifiers, one for die/oops and the other for panic. With the addition of panic notifier "id", this patch makes use of such "id" to unify both functions. It also comments the function and changes the priority of the notifier blocks, in order they run early compared to other notifiers, to prevent useless trace data (like the callback names for the other notifiers). Finally, we also removed an unnecessary header inclusion. Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- kernel/trace/trace.c | 57 +++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 25 deletions(-)