[PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[Huang Yiwei]

Currently ftrace only dumps the global trace buffer on an OOPs. For
debugging a production usecase, instance trace will be helpful to
check specific problems since global trace buffer may be used for
other purposes.

This patch extend the ftrace_dump_on_oops parameter to dump a specific
trace instance:

  - ftrace_dump_on_oops=0: as before -- don't dump
  - ftrace_dump_on_oops[=1]: as before -- dump the global trace buffer
  on all CPUs
  - ftrace_dump_on_oops=2 or =orig_cpu: as before -- dump the global
  trace buffer on CPU that triggered the oops
  - ftrace_dump_on_oops=<instance_name>: new behavior -- dump the
  tracing instance matching <instance_name>

Also, the sysctl node can handle the input accordingly.

Cc: Ross Zwisler <zwisler@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Signed-off-by: Huang Yiwei <quic_hyiwei@quicinc.com>
---
Link: https://lore.kernel.org/linux-trace-kernel/20221209125310.450aee97@gandalf.local.home/

 .../admin-guide/kernel-parameters.txt         |  9 +--
 Documentation/admin-guide/sysctl/kernel.rst   | 11 ++--
 include/linux/ftrace.h                        |  4 +-
 include/linux/kernel.h                        |  1 +
 kernel/sysctl.c                               |  4 +-
 kernel/trace/trace.c                          | 64 ++++++++++++++-----
 kernel/trace/trace_selftest.c                 |  2 +-
 7 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a36cf8cc582c..b3aa533253f1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1559,12 +1559,13 @@
 			The above will cause the "foo" tracing instance to trigger
 			a snapshot at the end of boot up.
 
-	ftrace_dump_on_oops[=orig_cpu]
+	ftrace_dump_on_oops[=orig_cpu | =<instance>]
 			[FTRACE] will dump the trace buffers on oops.
-			If no parameter is passed, ftrace will dump
-			buffers of all CPUs, but if you pass orig_cpu, it will
+			If no parameter is passed, ftrace will dump global
+			buffers of all CPUs, if you pass orig_cpu, it will
 			dump only the buffer of the CPU that triggered the
-			oops.
+			oops, or specific instance will be dumped if instance
+			name is passed.
 
 	ftrace_filter=[function-list]
 			[FTRACE] Limit the functions traced by the function
diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index 6584a1f9bfe3..ecf036b63c48 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -296,11 +296,12 @@ kernel panic). This will output the contents of the ftrace buffers to
 the console.  This is very useful for capturing traces that lead to
 crashes and outputting them to a serial console.
 
-= ===================================================
-0 Disabled (default).
-1 Dump buffers of all CPUs.
-2 Dump the buffer of the CPU that triggered the oops.
-= ===================================================
+=========== ===================================================
+0           Disabled (default).
+1           Dump buffers of all CPUs.
+2(orig_cpu) Dump the buffer of the CPU that triggered the oops.
+<instance>  Dump the specific instance buffer.
+=========== ===================================================
 
 
 ftrace_enabled, stack_tracer_enabled
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index e8921871ef9a..f20de4621ec1 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1151,7 +1151,9 @@ static inline void unpause_graph_tracing(void) { }
 #ifdef CONFIG_TRACING
 enum ftrace_dump_mode;
 
-extern enum ftrace_dump_mode ftrace_dump_on_oops;
+#define MAX_TRACER_SIZE		100
+extern char ftrace_dump_on_oops[];
+extern enum ftrace_dump_mode get_ftrace_dump_mode(void);
 extern int tracepoint_printk;
 
 extern void disable_trace_on_warning(void);
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d9ad21058eed..de4a76036372 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -255,6 +255,7 @@ enum ftrace_dump_mode {
 	DUMP_NONE,
 	DUMP_ALL,
 	DUMP_ORIG,
+	DUMP_INSTANCE,
 };
 
 #ifdef CONFIG_TRACING
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 157f7ce2942d..81cc974913bb 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1710,9 +1710,9 @@ static struct ctl_table kern_table[] = {
 	{
 		.procname	= "ftrace_dump_on_oops",
 		.data		= &ftrace_dump_on_oops,
-		.maxlen		= sizeof(int),
+		.maxlen		= MAX_TRACER_SIZE,
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dostring,
 	},
 	{
 		.procname	= "traceoff_on_warning",
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a0defe156b57..9a76a278e5c3 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -130,9 +130,10 @@ cpumask_var_t __read_mostly	tracing_buffer_mask;
  * /proc/sys/kernel/ftrace_dump_on_oops
  * Set 1 if you want to dump buffers of all CPUs
  * Set 2 if you want to dump the buffer of the CPU that triggered oops
+ * Set instance name if you want to dump the specific trace instance
  */
-
-enum ftrace_dump_mode ftrace_dump_on_oops;
+/* Set to string format zero to disable by default */
+char ftrace_dump_on_oops[MAX_TRACER_SIZE] = "0";
 
 /* When set, tracing will stop when a WARN*() is hit */
 int __disable_trace_on_warning;
@@ -178,7 +179,6 @@ static void ftrace_trace_userstack(struct trace_array *tr,
 				   struct trace_buffer *buffer,
 				   unsigned int trace_ctx);
 
-#define MAX_TRACER_SIZE		100
 static char bootup_tracer_buf[MAX_TRACER_SIZE] __initdata;
 static char *default_bootup_tracer;
 
@@ -201,19 +201,32 @@ static int __init set_cmdline_ftrace(char *str)
 }
 __setup("ftrace=", set_cmdline_ftrace);
 
+enum ftrace_dump_mode get_ftrace_dump_mode(void)
+{
+	if (!strcmp("0", ftrace_dump_on_oops))
+		return DUMP_NONE;
+	else if (!strcmp("1", ftrace_dump_on_oops))
+		return DUMP_ALL;
+	else if (!strcmp("orig_cpu", ftrace_dump_on_oops) ||
+			!strcmp("2", ftrace_dump_on_oops))
+		return DUMP_ORIG;
+	else
+		return DUMP_INSTANCE;
+}
+
 static int __init set_ftrace_dump_on_oops(char *str)
 {
-	if (*str++ != '=' || !*str || !strcmp("1", str)) {
-		ftrace_dump_on_oops = DUMP_ALL;
+	if (!*str) {
+		strscpy(ftrace_dump_on_oops, "1", MAX_TRACER_SIZE);
 		return 1;
 	}
 
-	if (!strcmp("orig_cpu", str) || !strcmp("2", str)) {
-		ftrace_dump_on_oops = DUMP_ORIG;
-                return 1;
-        }
+	if (*str++ == '=') {
+		strscpy(ftrace_dump_on_oops, str, MAX_TRACER_SIZE);
+		return 1;
+	}
 
-        return 0;
+	return 0;
 }
 __setup("ftrace_dump_on_oops", set_ftrace_dump_on_oops);
 
@@ -10085,14 +10098,16 @@ static struct notifier_block trace_die_notifier = {
 static int trace_die_panic_handler(struct notifier_block *self,
 				unsigned long ev, void *unused)
 {
-	if (!ftrace_dump_on_oops)
+	enum ftrace_dump_mode dump_mode = get_ftrace_dump_mode();
+
+	if (!dump_mode)
 		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);
+	ftrace_dump(dump_mode);
 
 	return NOTIFY_DONE;
 }
@@ -10133,12 +10148,12 @@ trace_printk_seq(struct trace_seq *s)
 	trace_seq_init(s);
 }
 
-void trace_init_global_iter(struct trace_iterator *iter)
+static void trace_init_iter(struct trace_iterator *iter, struct trace_array *tr)
 {
-	iter->tr = &global_trace;
+	iter->tr = tr;
 	iter->trace = iter->tr->current_trace;
 	iter->cpu_file = RING_BUFFER_ALL_CPUS;
-	iter->array_buffer = &global_trace.array_buffer;
+	iter->array_buffer = &tr->array_buffer;
 
 	if (iter->trace && iter->trace->open)
 		iter->trace->open(iter);
@@ -10158,6 +10173,11 @@ void trace_init_global_iter(struct trace_iterator *iter)
 	iter->fmt_size = STATIC_FMT_BUF_SIZE;
 }
 
+void trace_init_global_iter(struct trace_iterator *iter)
+{
+	trace_init_iter(iter, &global_trace);
+}
+
 void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 {
 	/* use static because iter can be a bit big for the stack */
@@ -10168,6 +10188,15 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 	unsigned long flags;
 	int cnt = 0, cpu;
 
+	if (oops_dump_mode == DUMP_INSTANCE) {
+		tr = trace_array_find(ftrace_dump_on_oops);
+		if (!tr) {
+			printk(KERN_TRACE "Instance %s not found\n",
+				ftrace_dump_on_oops);
+			return;
+		}
+	}
+
 	/* Only allow one dump user at a time. */
 	if (atomic_inc_return(&dump_running) != 1) {
 		atomic_dec(&dump_running);
@@ -10182,12 +10211,12 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 	 * If the user does a sysrq-z, then they can re-enable
 	 * tracing with echo 1 > tracing_on.
 	 */
-	tracing_off();
+	tracer_tracing_off(tr);
 
 	local_irq_save(flags);
 
 	/* Simulate the iterator */
-	trace_init_global_iter(&iter);
+	trace_init_iter(&iter, tr);
 
 	for_each_tracing_cpu(cpu) {
 		atomic_inc(&per_cpu_ptr(iter.array_buffer->data, cpu)->disabled);
@@ -10200,6 +10229,7 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 
 	switch (oops_dump_mode) {
 	case DUMP_ALL:
+	case DUMP_INSTANCE:
 		iter.cpu_file = RING_BUFFER_ALL_CPUS;
 		break;
 	case DUMP_ORIG:
diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index 529590499b1f..a9750a680324 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -768,7 +768,7 @@ static int trace_graph_entry_watchdog(struct ftrace_graph_ent *trace)
 	if (unlikely(++graph_hang_thresh > GRAPH_MAX_FUNC_TEST)) {
 		ftrace_graph_stop();
 		printk(KERN_WARNING "BUG: Function graph tracer hang!\n");
-		if (ftrace_dump_on_oops) {
+		if (get_ftrace_dump_mode()) {
 			ftrace_dump(DUMP_ALL);
 			/* ftrace_dump() disables tracing */
 			tracing_on();
-- 
2.25.1

Re: [PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[Steven Rostedt]

On Fri, 19 Jan 2024 16:08:24 +0800
Huang Yiwei <quic_hyiwei@quicinc.com> wrote:

> -	ftrace_dump_on_oops[=orig_cpu]
> +	ftrace_dump_on_oops[=orig_cpu | =<instance>]

I wonder if we should have it be:

	ftrace_dump_on_oops[=orig_cpu | =<instance> | =<instance>:orig_cpu ]

Then last would be to only print out a specific CPU trace of the given instance.

And if we really want to be fancy!

	ftrace_dump_on_opps[=orig_cpu | =<instance> | =orig_cpu:<instance> ][,<instance> | ,<instance>:orig_cpu]

That would allow dumping more than one instance.

If you want to dump the main buffer and an instance foo:

	ftrace_dump_on_opps,foo

Where the ',' says to dump the top instance as well as the foo instance.

-- Steve


>  			[FTRACE] will dump the trace buffers on oops.
> -			If no parameter is passed, ftrace will dump
> -			buffers of all CPUs, but if you pass orig_cpu, it will
> +			If no parameter is passed, ftrace will dump global
> +			buffers of all CPUs, if you pass orig_cpu, it will
>  			dump only the buffer of the CPU that triggered the
> -			oops.
> +			oops, or specific instance will be dumped if instance
> +			name is passed.
>  

Re: [PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[Huang Yiwei]

On 1/20/2024 12:56 AM, Steven Rostedt wrote:
> On Fri, 19 Jan 2024 16:08:24 +0800
> Huang Yiwei <quic_hyiwei@quicinc.com> wrote:
> 
>> -	ftrace_dump_on_oops[=orig_cpu]
>> +	ftrace_dump_on_oops[=orig_cpu | =<instance>]
> 
> I wonder if we should have it be:
> 
> 	ftrace_dump_on_oops[=orig_cpu | =<instance> | =<instance>:orig_cpu ]
> 
> Then last would be to only print out a specific CPU trace of the given instance.
> 
> And if we really want to be fancy!
> 
> 	ftrace_dump_on_opps[=orig_cpu | =<instance> | =orig_cpu:<instance> ][,<instance> | ,<instance>:orig_cpu]
> 
Yeah, I agree to make the parameter more flexible.

"=orig_cpu:<instance>" means to dump global and another instance?

I'm thinking of the following format:

ftrace_dump_on_opps[=orig_cpu | =<instance>][,<instance> | 
,<instance>=orig_cpu]

Here list some possible situations:

1. Dump global on orig_cpu:
ftrace_dump_on_oops=orig_cpu

2. Dump global and instance1 on all cpu, instance2 on orig_cpu:
ftrace_dump_on_opps,<instance1>,<instance2>=orig_cpu

3. Dump global and instance1 on orig_cpu, instance2 on all cpu:
ftrace_dump_on_opps=orig_cpu,<instance1>=orig_cpu,<instance2>

4. Dump instance1 on all cpu, instance2 on orig_cpu:
ftrace_dump_on_opps=<instance1>,<instance2>=orig_cpu

5. Dump instance1 and instance2 on orig_cpu:
ftrace_dump_on_opps=<instance1>=orig_cpu,<instance2>=orig_cpu

This makes orig_cpu dump for global same as instance, the parameter may 
seems more unified and users don't need to remember another markers to 
request orig_cpu dump.

But one problem here is if there's an instance named "orig_cpu", then we 
may not dump it correctly.

Regards,
Huang Yiwei

> That would allow dumping more than one instance.
> 
> If you want to dump the main buffer and an instance foo:
> 
> 	ftrace_dump_on_opps,foo
> 
> Where the ',' says to dump the top instance as well as the foo instance.
> 
> -- Steve
> 
> 
>>   			[FTRACE] will dump the trace buffers on oops.
>> -			If no parameter is passed, ftrace will dump
>> -			buffers of all CPUs, but if you pass orig_cpu, it will
>> +			If no parameter is passed, ftrace will dump global
>> +			buffers of all CPUs, if you pass orig_cpu, it will
>>   			dump only the buffer of the CPU that triggered the
>> -			oops.
>> +			oops, or specific instance will be dumped if instance
>> +			name is passed.
>>   

Re: [PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[Steven Rostedt]

On Tue, 23 Jan 2024 18:23:58 +0800
Huang Yiwei <quic_hyiwei@quicinc.com> wrote:

> > And if we really want to be fancy!
> > 
> > 	ftrace_dump_on_opps[=orig_cpu | =<instance> | =orig_cpu:<instance> ][,<instance> | ,<instance>:orig_cpu]
> >   
> Yeah, I agree to make the parameter more flexible.
> 
> "=orig_cpu:<instance>" means to dump global and another instance?

No, I added a comma for that:

  =,orig_cpu:<instance>

Would mean to dump all of global and just the origin CPU of the instance.

> 
> I'm thinking of the following format:
> 
> ftrace_dump_on_opps[=orig_cpu | =<instance>][,<instance> | 
> ,<instance>=orig_cpu]
> 
> Here list some possible situations:
> 
> 1. Dump global on orig_cpu:
> ftrace_dump_on_oops=orig_cpu
> 
> 2. Dump global and instance1 on all cpu, instance2 on orig_cpu:
> ftrace_dump_on_opps,<instance1>,<instance2>=orig_cpu
> 
> 3. Dump global and instance1 on orig_cpu, instance2 on all cpu:
> ftrace_dump_on_opps=orig_cpu,<instance1>=orig_cpu,<instance2>
> 
> 4. Dump instance1 on all cpu, instance2 on orig_cpu:
> ftrace_dump_on_opps=<instance1>,<instance2>=orig_cpu
> 
> 5. Dump instance1 and instance2 on orig_cpu:
> ftrace_dump_on_opps=<instance1>=orig_cpu,<instance2>=orig_cpu
> 
> This makes orig_cpu dump for global same as instance, the parameter may 
> seems more unified and users don't need to remember another markers to 
> request orig_cpu dump.
> 
> But one problem here is if there's an instance named "orig_cpu", then we 
> may not dump it correctly.

I would put that under:

   Patient: Doctor it hurts me when I do this
   Doctor:  Then don't do that

;-)

-- Steve

Re: [PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[Steven Rostedt]

On Tue, 23 Jan 2024 09:49:00 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 23 Jan 2024 18:23:58 +0800
> Huang Yiwei <quic_hyiwei@quicinc.com> wrote:
> 
> > > And if we really want to be fancy!
> > > 
> > > 	ftrace_dump_on_opps[=orig_cpu | =<instance> | =orig_cpu:<instance> ][,<instance> | ,<instance>:orig_cpu]
> > >     
> > Yeah, I agree to make the parameter more flexible.
> > 
> > "=orig_cpu:<instance>" means to dump global and another instance?  
> 
> No, I added a comma for that:
> 
>   =,orig_cpu:<instance>
> 
> Would mean to dump all of global and just the origin CPU of the instance.
> 
> > 
> > I'm thinking of the following format:
> > 
> > ftrace_dump_on_opps[=orig_cpu | =<instance>][,<instance> | 
> > ,<instance>=orig_cpu]
> > 
> > Here list some possible situations:
> > 
> > 1. Dump global on orig_cpu:
> > ftrace_dump_on_oops=orig_cpu
> > 
> > 2. Dump global and instance1 on all cpu, instance2 on orig_cpu:
> > ftrace_dump_on_opps,<instance1>,<instance2>=orig_cpu
> > 
> > 3. Dump global and instance1 on orig_cpu, instance2 on all cpu:
> > ftrace_dump_on_opps=orig_cpu,<instance1>=orig_cpu,<instance2>
> > 
> > 4. Dump instance1 on all cpu, instance2 on orig_cpu:
> > ftrace_dump_on_opps=<instance1>,<instance2>=orig_cpu
> > 
> > 5. Dump instance1 and instance2 on orig_cpu:
> > ftrace_dump_on_opps=<instance1>=orig_cpu,<instance2>=orig_cpu
> > 
> > This makes orig_cpu dump for global same as instance, the parameter may 
> > seems more unified and users don't need to remember another markers to 
> > request orig_cpu dump.
> > 
> > But one problem here is if there's an instance named "orig_cpu", then we 
> > may not dump it correctly.  
> 
> I would put that under:
> 
>    Patient: Doctor it hurts me when I do this
>    Doctor:  Then don't do that
> 
> ;-)

Oh, and I'm fine with what you proposed above.

-- Steve

Re: [PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 11185 bytes --]

On Fri, Jan 19, 2024 at 04:08:24PM +0800, Huang Yiwei wrote:
> Currently ftrace only dumps the global trace buffer on an OOPs. For
> debugging a production usecase, instance trace will be helpful to
> check specific problems since global trace buffer may be used for
> other purposes.
> 
> This patch extend the ftrace_dump_on_oops parameter to dump a specific
> trace instance:
> 
>   - ftrace_dump_on_oops=0: as before -- don't dump
>   - ftrace_dump_on_oops[=1]: as before -- dump the global trace buffer
>   on all CPUs
>   - ftrace_dump_on_oops=2 or =orig_cpu: as before -- dump the global
>   trace buffer on CPU that triggered the oops
>   - ftrace_dump_on_oops=<instance_name>: new behavior -- dump the
>   tracing instance matching <instance_name>
> 
> Also, the sysctl node can handle the input accordingly.
> 
> Cc: Ross Zwisler <zwisler@google.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Signed-off-by: Huang Yiwei <quic_hyiwei@quicinc.com>
> ---
> Link: https://lore.kernel.org/linux-trace-kernel/20221209125310.450aee97@gandalf.local.home/
> 
>  .../admin-guide/kernel-parameters.txt         |  9 +--
>  Documentation/admin-guide/sysctl/kernel.rst   | 11 ++--
>  include/linux/ftrace.h                        |  4 +-
>  include/linux/kernel.h                        |  1 +
>  kernel/sysctl.c                               |  4 +-
>  kernel/trace/trace.c                          | 64 ++++++++++++++-----
>  kernel/trace/trace_selftest.c                 |  2 +-
>  7 files changed, 65 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a36cf8cc582c..b3aa533253f1 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1559,12 +1559,13 @@
>  			The above will cause the "foo" tracing instance to trigger
>  			a snapshot at the end of boot up.
>  
> -	ftrace_dump_on_oops[=orig_cpu]
> +	ftrace_dump_on_oops[=orig_cpu | =<instance>]
>  			[FTRACE] will dump the trace buffers on oops.
> -			If no parameter is passed, ftrace will dump
> -			buffers of all CPUs, but if you pass orig_cpu, it will
> +			If no parameter is passed, ftrace will dump global
> +			buffers of all CPUs, if you pass orig_cpu, it will
>  			dump only the buffer of the CPU that triggered the
> -			oops.
> +			oops, or specific instance will be dumped if instance
> +			name is passed.
>  
>  	ftrace_filter=[function-list]
>  			[FTRACE] Limit the functions traced by the function
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 6584a1f9bfe3..ecf036b63c48 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -296,11 +296,12 @@ kernel panic). This will output the contents of the ftrace buffers to
>  the console.  This is very useful for capturing traces that lead to
>  crashes and outputting them to a serial console.
>  
> -= ===================================================
> -0 Disabled (default).
> -1 Dump buffers of all CPUs.
> -2 Dump the buffer of the CPU that triggered the oops.
> -= ===================================================
> +=========== ===================================================
> +0           Disabled (default).
> +1           Dump buffers of all CPUs.
> +2(orig_cpu) Dump the buffer of the CPU that triggered the oops.
> +<instance>  Dump the specific instance buffer.
> +=========== ===================================================
>  
>  
>  ftrace_enabled, stack_tracer_enabled
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index e8921871ef9a..f20de4621ec1 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1151,7 +1151,9 @@ static inline void unpause_graph_tracing(void) { }
>  #ifdef CONFIG_TRACING
>  enum ftrace_dump_mode;
>  
> -extern enum ftrace_dump_mode ftrace_dump_on_oops;
> +#define MAX_TRACER_SIZE		100
> +extern char ftrace_dump_on_oops[];
> +extern enum ftrace_dump_mode get_ftrace_dump_mode(void);
>  extern int tracepoint_printk;
>  
>  extern void disable_trace_on_warning(void);
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d9ad21058eed..de4a76036372 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -255,6 +255,7 @@ enum ftrace_dump_mode {
>  	DUMP_NONE,
>  	DUMP_ALL,
>  	DUMP_ORIG,
> +	DUMP_INSTANCE,
>  };
>  
>  #ifdef CONFIG_TRACING
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 157f7ce2942d..81cc974913bb 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1710,9 +1710,9 @@ static struct ctl_table kern_table[] = {
>  	{
>  		.procname	= "ftrace_dump_on_oops",
>  		.data		= &ftrace_dump_on_oops,
> -		.maxlen		= sizeof(int),
> +		.maxlen		= MAX_TRACER_SIZE,
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler	= proc_dostring,
>  	},
>  	{
>  		.procname	= "traceoff_on_warning",
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index a0defe156b57..9a76a278e5c3 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -130,9 +130,10 @@ cpumask_var_t __read_mostly	tracing_buffer_mask;
>   * /proc/sys/kernel/ftrace_dump_on_oops
>   * Set 1 if you want to dump buffers of all CPUs
>   * Set 2 if you want to dump the buffer of the CPU that triggered oops
> + * Set instance name if you want to dump the specific trace instance
>   */
> -
> -enum ftrace_dump_mode ftrace_dump_on_oops;
> +/* Set to string format zero to disable by default */
> +char ftrace_dump_on_oops[MAX_TRACER_SIZE] = "0";
>  
>  /* When set, tracing will stop when a WARN*() is hit */
>  int __disable_trace_on_warning;
> @@ -178,7 +179,6 @@ static void ftrace_trace_userstack(struct trace_array *tr,
>  				   struct trace_buffer *buffer,
>  				   unsigned int trace_ctx);
>  
> -#define MAX_TRACER_SIZE		100
>  static char bootup_tracer_buf[MAX_TRACER_SIZE] __initdata;
>  static char *default_bootup_tracer;
>  
> @@ -201,19 +201,32 @@ static int __init set_cmdline_ftrace(char *str)
>  }
>  __setup("ftrace=", set_cmdline_ftrace);
>  
> +enum ftrace_dump_mode get_ftrace_dump_mode(void)
> +{
> +	if (!strcmp("0", ftrace_dump_on_oops))
Would using a strncmp be better in this case? And this question goes for
all the strcmp in the patch. Something like strncmp("0",
ftrace_dump_on_oops, 1); when they are equal, it would avoid 2
assignments and two comparisons. Also might avoid runaway comparisons if
the first string constant changes in the future.

Or maybe strncmp("0", ftrace_dump_on_oops, 2); if you want to check if
they are both null terminated.

> +		return DUMP_NONE;
> +	else if (!strcmp("1", ftrace_dump_on_oops))
> +		return DUMP_ALL;
> +	else if (!strcmp("orig_cpu", ftrace_dump_on_oops) ||
> +			!strcmp("2", ftrace_dump_on_oops))
> +		return DUMP_ORIG;
> +	else
> +		return DUMP_INSTANCE;
> +}
> +
>  static int __init set_ftrace_dump_on_oops(char *str)
>  {
> -	if (*str++ != '=' || !*str || !strcmp("1", str)) {
> -		ftrace_dump_on_oops = DUMP_ALL;
> +	if (!*str) {
> +		strscpy(ftrace_dump_on_oops, "1", MAX_TRACER_SIZE);
>  		return 1;
>  	}
>  
> -	if (!strcmp("orig_cpu", str) || !strcmp("2", str)) {
> -		ftrace_dump_on_oops = DUMP_ORIG;
> -                return 1;
> -        }
> +	if (*str++ == '=') {
> +		strscpy(ftrace_dump_on_oops, str, MAX_TRACER_SIZE);
> +		return 1;
> +	}
>  
> -        return 0;
> +	return 0;
>  }
>  __setup("ftrace_dump_on_oops", set_ftrace_dump_on_oops);
>  
> @@ -10085,14 +10098,16 @@ static struct notifier_block trace_die_notifier = {
>  static int trace_die_panic_handler(struct notifier_block *self,
>  				unsigned long ev, void *unused)
>  {
> -	if (!ftrace_dump_on_oops)
> +	enum ftrace_dump_mode dump_mode = get_ftrace_dump_mode();
> +
> +	if (!dump_mode)
>  		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);
> +	ftrace_dump(dump_mode);
>  
>  	return NOTIFY_DONE;
>  }
> @@ -10133,12 +10148,12 @@ trace_printk_seq(struct trace_seq *s)
>  	trace_seq_init(s);
>  }
>  
> -void trace_init_global_iter(struct trace_iterator *iter)
> +static void trace_init_iter(struct trace_iterator *iter, struct trace_array *tr)
>  {
> -	iter->tr = &global_trace;
> +	iter->tr = tr;
>  	iter->trace = iter->tr->current_trace;
>  	iter->cpu_file = RING_BUFFER_ALL_CPUS;
> -	iter->array_buffer = &global_trace.array_buffer;
> +	iter->array_buffer = &tr->array_buffer;
>  
>  	if (iter->trace && iter->trace->open)
>  		iter->trace->open(iter);
> @@ -10158,6 +10173,11 @@ void trace_init_global_iter(struct trace_iterator *iter)
>  	iter->fmt_size = STATIC_FMT_BUF_SIZE;
>  }
>  
> +void trace_init_global_iter(struct trace_iterator *iter)
> +{
> +	trace_init_iter(iter, &global_trace);
> +}
> +
>  void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>  {
>  	/* use static because iter can be a bit big for the stack */
> @@ -10168,6 +10188,15 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>  	unsigned long flags;
>  	int cnt = 0, cpu;
>  
> +	if (oops_dump_mode == DUMP_INSTANCE) {
> +		tr = trace_array_find(ftrace_dump_on_oops);
> +		if (!tr) {
> +			printk(KERN_TRACE "Instance %s not found\n",
> +				ftrace_dump_on_oops);
> +			return;
> +		}
> +	}
> +
>  	/* Only allow one dump user at a time. */
>  	if (atomic_inc_return(&dump_running) != 1) {
>  		atomic_dec(&dump_running);
> @@ -10182,12 +10211,12 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>  	 * If the user does a sysrq-z, then they can re-enable
>  	 * tracing with echo 1 > tracing_on.
>  	 */
> -	tracing_off();
> +	tracer_tracing_off(tr);
>  
>  	local_irq_save(flags);
>  
>  	/* Simulate the iterator */
> -	trace_init_global_iter(&iter);
> +	trace_init_iter(&iter, tr);
>  
>  	for_each_tracing_cpu(cpu) {
>  		atomic_inc(&per_cpu_ptr(iter.array_buffer->data, cpu)->disabled);
> @@ -10200,6 +10229,7 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>  
>  	switch (oops_dump_mode) {
>  	case DUMP_ALL:
> +	case DUMP_INSTANCE:
>  		iter.cpu_file = RING_BUFFER_ALL_CPUS;
>  		break;
>  	case DUMP_ORIG:
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index 529590499b1f..a9750a680324 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -768,7 +768,7 @@ static int trace_graph_entry_watchdog(struct ftrace_graph_ent *trace)
>  	if (unlikely(++graph_hang_thresh > GRAPH_MAX_FUNC_TEST)) {
>  		ftrace_graph_stop();
>  		printk(KERN_WARNING "BUG: Function graph tracer hang!\n");
> -		if (ftrace_dump_on_oops) {
> +		if (get_ftrace_dump_mode()) {
>  			ftrace_dump(DUMP_ALL);
>  			/* ftrace_dump() disables tracing */
>  			tracing_on();
> -- 
> 2.25.1
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3] tracing: Support to dump instance traces by ftrace_dump_on_oops

[Bjorn Andersson]

On Mon, Jan 22, 2024 at 02:56:45PM +0100, Joel Granados wrote:
> On Fri, Jan 19, 2024 at 04:08:24PM +0800, Huang Yiwei wrote:
[..]
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
[..]
> > +enum ftrace_dump_mode get_ftrace_dump_mode(void)
> > +{
> > +	if (!strcmp("0", ftrace_dump_on_oops))
> Would using a strncmp be better in this case? And this question goes for
> all the strcmp in the patch. Something like strncmp("0",
> ftrace_dump_on_oops, 1); when they are equal, it would avoid 2
> assignments and two comparisons.

As you determine yourself below, Huang is looking for the string "0" not
just something with the first character being '0', so you you need to
check for null termination.

> Also might avoid runaway comparisons if
> the first string constant changes in the future.
> 

If the constant suddenly isn't null terminated, causing strcmp to run
"endlessly", we have bigger problems.

> Or maybe strncmp("0", ftrace_dump_on_oops, 2); if you want to check if
> they are both null terminated.
> 

This is just obscure. At best it would confuse future readers.

Regards,
Bjorn