[PATCH] tracing: Do not free snapshot if tracer is on cmdline

[PATCH] tracing: Do not free snapshot if tracer is on cmdline

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The ftrace_boot_snapshot and alloc_snapshot cmdline options allocate the
snapshot buffer at boot up for use later. The ftrace_boot_snapshot in
particular requires the snapshot to be allocated because it will take a
snapshot at the end of boot up allowing to see the traces that happened
during boot so that it's not lost when user space takes over.

When a tracer is registered (started) there's a path that checks if it
requires the snapshot buffer or not, and if it does not and it was
allocated it will do a synchronization and free the snapshot buffer.

This is only required if the previous tracer was using it for "max
latency" snapshots, as it needs to make sure all max snapshots are
complete before freeing. But this is only needed if the previous tracer
was using the snapshot buffer for latency (like irqoff tracer and
friends). But it does not make sense to free it, if the previous tracer
was not using it, and the snapshot was allocated by the cmdline
parameters. This basically takes away the point of allocating it in the
first place!

Note, the allocated snapshot worked fine for just trace events, but fails
when a tracer is enabled on the cmdline.

Cc: stable@vger.kernel.org
Fixes: 55034cd6e6481 ("tracing: Add alloc_snapshot kernel command line parameter")
Reported-by: Ross Zwisler <zwisler@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index def721de68a0..871e2b592969 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6432,7 +6432,7 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
 	tr->current_trace = &nop_trace;
 
 #ifdef CONFIG_TRACER_MAX_TRACE
-	had_max_tr = tr->allocated_snapshot;
+	had_max_tr = tr->current_trace->use_max_tr;
 
 	if (had_max_tr && !t->use_max_tr) {
 		/*
-- 
2.35.1

Re: [PATCH] tracing: Do not free snapshot if tracer is on cmdline

[Masami Hiramatsu]

On Tue, 4 Oct 2022 18:04:52 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The ftrace_boot_snapshot and alloc_snapshot cmdline options allocate the
> snapshot buffer at boot up for use later. The ftrace_boot_snapshot in
> particular requires the snapshot to be allocated because it will take a
> snapshot at the end of boot up allowing to see the traces that happened
> during boot so that it's not lost when user space takes over.
> 
> When a tracer is registered (started) there's a path that checks if it
> requires the snapshot buffer or not, and if it does not and it was
> allocated it will do a synchronization and free the snapshot buffer.
> 
> This is only required if the previous tracer was using it for "max
> latency" snapshots, as it needs to make sure all max snapshots are
> complete before freeing. But this is only needed if the previous tracer
> was using the snapshot buffer for latency (like irqoff tracer and
> friends). But it does not make sense to free it, if the previous tracer
> was not using it, and the snapshot was allocated by the cmdline
> parameters. This basically takes away the point of allocating it in the
> first place!
> 
> Note, the allocated snapshot worked fine for just trace events, but fails
> when a tracer is enabled on the cmdline.
> 
> Cc: stable@vger.kernel.org
> Fixes: 55034cd6e6481 ("tracing: Add alloc_snapshot kernel command line parameter")
> Reported-by: Ross Zwisler <zwisler@kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

> ---
>  kernel/trace/trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index def721de68a0..871e2b592969 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6432,7 +6432,7 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
>  	tr->current_trace = &nop_trace;
>  
>  #ifdef CONFIG_TRACER_MAX_TRACE
> -	had_max_tr = tr->allocated_snapshot;
> +	had_max_tr = tr->current_trace->use_max_tr;
>  
>  	if (had_max_tr && !t->use_max_tr) {
>  		/*
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] tracing: Do not free snapshot if tracer is on cmdline

[Steven Rostedt]

On Wed, 5 Oct 2022 22:08:56 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Looks good to me.
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks!

Unfortunately, this is causing a failure in one of my tests. I'm still
trying to figure out how it is an issue.

-- Steve

> 
> > ---
> >  kernel/trace/trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index def721de68a0..871e2b592969 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -6432,7 +6432,7 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
> >  	tr->current_trace = &nop_trace;
> >  
> >  #ifdef CONFIG_TRACER_MAX_TRACE
> > -	had_max_tr = tr->allocated_snapshot;
> > +	had_max_tr = tr->current_trace->use_max_tr;
> >  
> >  	if (had_max_tr && !t->use_max_tr) {
> >  		/*