[PATCH] tracing: Update snapshot buffer on resize if it is allocated

[PATCH] tracing: Update snapshot buffer on resize if it is allocated

[Steven Rostedt]

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

The snapshot buffer is to mimic the main buffer so that when a snapshot is
needed, the snapshot and main buffer are swapped. When the snapshot buffer
is allocated, it is set to the minimal size that the ring buffer may be at
and still functional. When it is allocated it becomes the same size as the
main ring buffer, and when the main ring buffer changes in size, it should
do.

Currently, the resize only updates the snapshot buffer if it's used by the
current tracer (ie. the preemptirqsoff tracer). But it needs to be updated
anytime it is allocated.

When changing the size of the main buffer, instead of looking to see if
the current tracer is utilizing the snapshot buffer, just check if it is
allocated to know if it should be updated or not.

Also fix typo in comment just above the code change.

Cc: stable@vger.kernel.org
Fixes: ad909e21bbe69 ("tracing: Add internal tracing_snapshot() functions")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index aa8f99f3e5de..6c79548f9574 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6348,7 +6348,7 @@ static int __tracing_resize_ring_buffer(struct trace_array *tr,
 	if (!tr->array_buffer.buffer)
 		return 0;
 
-	/* Do not allow tracing while resizng ring buffer */
+	/* Do not allow tracing while resizing ring buffer */
 	tracing_stop_tr(tr);
 
 	ret = ring_buffer_resize(tr->array_buffer.buffer, size, cpu);
@@ -6356,7 +6356,7 @@ static int __tracing_resize_ring_buffer(struct trace_array *tr,
 		goto out_start;
 
 #ifdef CONFIG_TRACER_MAX_TRACE
-	if (!tr->current_trace->use_max_tr)
+	if (!tr->allocated_snapshot)
 		goto out;
 
 	ret = ring_buffer_resize(tr->max_buffer.buffer, size, cpu);
-- 
2.42.0

Re: [PATCH] tracing: Update snapshot buffer on resize if it is allocated

[Masami Hiramatsu]

On Sun, 10 Dec 2023 22:54:47 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The snapshot buffer is to mimic the main buffer so that when a snapshot is
> needed, the snapshot and main buffer are swapped. When the snapshot buffer
> is allocated, it is set to the minimal size that the ring buffer may be at
> and still functional. When it is allocated it becomes the same size as the
> main ring buffer, and when the main ring buffer changes in size, it should
> do.

nit: There seems two "when the snapshot buffer is allocated" case, maybe latter
"it" means main buffer?

> 
> Currently, the resize only updates the snapshot buffer if it's used by the
> current tracer (ie. the preemptirqsoff tracer). But it needs to be updated
> anytime it is allocated.
> 
> When changing the size of the main buffer, instead of looking to see if
> the current tracer is utilizing the snapshot buffer, just check if it is
> allocated to know if it should be updated or not.
> 
> Also fix typo in comment just above the code change.
> 

Looks good to me.

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

BTW, the historical naming leads this kind of issues.
Maybe we'd better to rename 'max_buffer' to 'snapshot_buffer'?

Thanks,


> Cc: stable@vger.kernel.org
> Fixes: ad909e21bbe69 ("tracing: Add internal tracing_snapshot() functions")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index aa8f99f3e5de..6c79548f9574 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6348,7 +6348,7 @@ static int __tracing_resize_ring_buffer(struct trace_array *tr,
>  	if (!tr->array_buffer.buffer)
>  		return 0;
>  
> -	/* Do not allow tracing while resizng ring buffer */
> +	/* Do not allow tracing while resizing ring buffer */
>  	tracing_stop_tr(tr);
>  
>  	ret = ring_buffer_resize(tr->array_buffer.buffer, size, cpu);
> @@ -6356,7 +6356,7 @@ static int __tracing_resize_ring_buffer(struct trace_array *tr,
>  		goto out_start;
>  
>  #ifdef CONFIG_TRACER_MAX_TRACE
> -	if (!tr->current_trace->use_max_tr)
> +	if (!tr->allocated_snapshot)
>  		goto out;
>  
>  	ret = ring_buffer_resize(tr->max_buffer.buffer, size, cpu);
> -- 
> 2.42.0
> 


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

Re: [PATCH] tracing: Update snapshot buffer on resize if it is allocated

[Steven Rostedt]

On Mon, 11 Dec 2023 21:31:34 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Sun, 10 Dec 2023 22:54:47 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > The snapshot buffer is to mimic the main buffer so that when a snapshot is
> > needed, the snapshot and main buffer are swapped. When the snapshot buffer
> > is allocated, it is set to the minimal size that the ring buffer may be at
> > and still functional. When it is allocated it becomes the same size as the
> > main ring buffer, and when the main ring buffer changes in size, it should
> > do.  
> 
> nit: There seems two "when the snapshot buffer is allocated" case, maybe latter
> "it" means main buffer?

I changed the paragraph to be:

    The snapshot buffer is to mimic the main buffer so that when a snapshot is
    needed, the snapshot and main buffer are swapped. When the snapshot buffer
    is allocated, it is set to the minimal size that the ring buffer may be at
    and still functional. When it is allocated it becomes the same size as the
    main ring buffer, and when the main ring buffer changes in size, the
    snapshot should also change in size if it is allocated.

> 
> > 
> > Currently, the resize only updates the snapshot buffer if it's used by the
> > current tracer (ie. the preemptirqsoff tracer). But it needs to be updated
> > anytime it is allocated.
> > 
> > When changing the size of the main buffer, instead of looking to see if
> > the current tracer is utilizing the snapshot buffer, just check if it is
> > allocated to know if it should be updated or not.
> > 
> > Also fix typo in comment just above the code change.
> >   
> 
> Looks good to me.
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks!

> 
> BTW, the historical naming leads this kind of issues.
> Maybe we'd better to rename 'max_buffer' to 'snapshot_buffer'?

Agreed. But that's a cleanup for another day. Hmm, maybe that too should be
marked as "KTODO"?

  https://lore.kernel.org/all/369bc919-1a1d-4f37-9cc9-742a86a41282@kadam.mountain/


There's a lot of things that we have been discussing on these ring-buffer
patches that could be KTODO items.

-- Steve

Re: [PATCH] tracing: Update snapshot buffer on resize if it is allocated

[Masami Hiramatsu]

On Mon, 11 Dec 2023 12:51:52 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 11 Dec 2023 21:31:34 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Sun, 10 Dec 2023 22:54:47 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > > 
> > > The snapshot buffer is to mimic the main buffer so that when a snapshot is
> > > needed, the snapshot and main buffer are swapped. When the snapshot buffer
> > > is allocated, it is set to the minimal size that the ring buffer may be at
> > > and still functional. When it is allocated it becomes the same size as the
> > > main ring buffer, and when the main ring buffer changes in size, it should
> > > do.  
> > 
> > nit: There seems two "when the snapshot buffer is allocated" case, maybe latter
> > "it" means main buffer?
> 
> I changed the paragraph to be:
> 
>     The snapshot buffer is to mimic the main buffer so that when a snapshot is
>     needed, the snapshot and main buffer are swapped. When the snapshot buffer
>     is allocated, it is set to the minimal size that the ring buffer may be at
>     and still functional. When it is allocated it becomes the same size as the
>     main ring buffer, and when the main ring buffer changes in size, the
>     snapshot should also change in size if it is allocated.

Yeah, this makes clearer.

> 
> > 
> > > 
> > > Currently, the resize only updates the snapshot buffer if it's used by the
> > > current tracer (ie. the preemptirqsoff tracer). But it needs to be updated
> > > anytime it is allocated.
> > > 
> > > When changing the size of the main buffer, instead of looking to see if
> > > the current tracer is utilizing the snapshot buffer, just check if it is
> > > allocated to know if it should be updated or not.
> > > 
> > > Also fix typo in comment just above the code change.
> > >   
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Thanks!
> 
> > 
> > BTW, the historical naming leads this kind of issues.
> > Maybe we'd better to rename 'max_buffer' to 'snapshot_buffer'?
> 
> Agreed. But that's a cleanup for another day. Hmm, maybe that too should be
> marked as "KTODO"?

Yes!

> 
>   https://lore.kernel.org/all/369bc919-1a1d-4f37-9cc9-742a86a41282@kadam.mountain/
> 

Ah, this is a good idea.

> 
> There's a lot of things that we have been discussing on these ring-buffer
> patches that could be KTODO items.

Thanks,

> 
> -- Steve


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