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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20231212081413.c4710c1b643ac4e0b975a122@kernel.org>
Date:   Tue, 12 Dec 2023 08:14:13 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux trace kernel <linux-trace-kernel@...r.kernel.org>,
        Mark Rutland <mark.rutland@....com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Dan Carpenter <dan.carpenter@...aro.org>,
        outreachy@...ts.linux.dev, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] tracing: Update snapshot buffer on resize if it is
 allocated

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

> On Mon, 11 Dec 2023 21:31:34 +0900
> Masami Hiramatsu (Google) <mhiramat@...nel.org> wrote:
> 
> > On Sun, 10 Dec 2023 22:54:47 -0500
> > Steven Rostedt <rostedt@...dmis.org> wrote:
> > 
> > > From: "Steven Rostedt (Google)" <rostedt@...dmis.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@...nel.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@...nel.org>