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

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

On Wed, 5 Oct 2022 11:37:57 -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.
> 
> Further investigation, this goes back even further and it does not require
> a tracer on the cmdline to fail. Simply enable snapshots and then enable a
> tracer, and it will remove the snapshot.
> 

This looks good to me :)

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

Thanks!

> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: stable@vger.kernel.org
> Fixes: 45ad21ca5530 ("tracing: Have trace_array keep track if snapshot buffer is allocated")
> Reported-by: Ross Zwisler <zwisler@kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lkml.kernel.org/r/20221004180452.6cf967f8@gandalf.local.home
>   - Didn't notice that tr->current_trace was set to &nop_trace just before
>     testing if the last tracer had use_max_tr. Which would always be false
>     as it was testing the nop_trace.use_max_tr which is false.
> 
>  kernel/trace/trace.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index def721de68a0..47a44b055a1d 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6428,12 +6428,12 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
>  	if (tr->current_trace->reset)
>  		tr->current_trace->reset(tr);
>  
> +#ifdef CONFIG_TRACER_MAX_TRACE
> +	had_max_tr = tr->current_trace->use_max_tr;
> +
>  	/* Current trace needs to be nop_trace before synchronize_rcu */
>  	tr->current_trace = &nop_trace;
>  
> -#ifdef CONFIG_TRACER_MAX_TRACE
> -	had_max_tr = tr->allocated_snapshot;
> -
>  	if (had_max_tr && !t->use_max_tr) {
>  		/*
>  		 * We need to make sure that the update_max_tr sees that
> @@ -6446,11 +6446,13 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
>  		free_snapshot(tr);
>  	}
>  
> -	if (t->use_max_tr && !had_max_tr) {
> +	if (t->use_max_tr && !tr->allocated_snapshot) {
>  		ret = tracing_alloc_snapshot_instance(tr);
>  		if (ret < 0)
>  			goto out;
>  	}
> +#else
> +	tr->current_trace = &nop_trace;
>  #endif
>  
>  	if (t->init) {
> -- 
> 2.35.1
> 


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