Re: [PATCH 17/30] tracing: Improve panic/die notifiers
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!
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
On 29/04/2022 10:23, Steven Rostedt wrote:
> On Fri, 29 Apr 2022 12:22:44 +0300
> Sergei Shtylyov 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)
>
> -- 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
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
On Fri, 29 Apr 2022 10:46:35 -0300 "Guilherme G. Piccoli" 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 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
On Fri, 29 Apr 2022 12:22:44 +0300
Sergei Shtylyov 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)
-- Steve
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
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 =) ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
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
> ---
> 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
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
