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 <[email protected]>
---
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
@@ -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,
};
/*
--
2.36.0
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 29/04/2022 10:23, Steven Rostedt wrote:
> On Fri, 29 Apr 2022 12:22:44 +0300
> Sergei Shtylyov <[email protected]> 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) <[email protected]>
>
> -- 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 12:22:44 +0300
Sergei Shtylyov <[email protected]> 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) <[email protected]>
-- Steve
On Fri, 29 Apr 2022 10:46:35 -0300
"Guilherme G. Piccoli" <[email protected]> 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
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 <[email protected]>
> ---
> 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 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!