linux-kernel - Re: [PATCH] docs/core-api: Fix circular buffer examples

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: <f4c744ec-0f68-49e2-93ff-8d7461cce2fc@paulmck-laptop>
Date: Wed, 6 Aug 2025 10:19:55 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Carlos Bilbao <bilbao@...edu>
Cc: Akira Yokosawa <akiyks@...il.com>, carlos.bilbao@...nel.org,
	corbet@....net, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, David Howells <dhowells@...hat.com>
Subject: Re: [PATCH] docs/core-api: Fix circular buffer examples

On Mon, Aug 04, 2025 at 05:05:30PM -0500, Carlos Bilbao wrote:
> Hello,
> 
> On 7/23/25 15:47, Paul E. McKenney wrote:
> > On Mon, Jul 21, 2025 at 11:47:25AM +0900, Akira Yokosawa wrote:
> > > +CC David and Paul, who are the authors of this doc.
> > > 
> > > On Sun, 20 Jul 2025 11:02:43 -0500, Carlos Bilbao wrote:
> > > > From: Carlos Bilbao <carlos.bilbao@...nel.org>
> > > > 
> > > > Fix circular buffer usage in producer/consumer examples in
> > > > circular-buffers.rst. They incorrectly access items using buffer[head] and
> > > > buffer[tail], as if buffer was a flat array; but the examples also use
> > > > buffer->head and buffer->tail, so it's a struct. Use buffer->vals[head] and
> > > > buffer->vals[tail] instead to match the intended layout.>
> > > > 
> > > > Signed-off-by: Carlos Bilbao <carlos.bilbao@...nel.org>
> > Hello, Carlos, and thank you for your attention to detail!
> > 
> > This one could likely use more help, as the last substantive change was
> > more than ten years ago.
> > 
> > But are you referring to a particular use of CIRC_SPACE() and CIRC_CNT()
> > for this change?  If so, could you please identify it in the commit log?
> 
> No, it's just the uses of the structure. Take a look at the patch, you'll
> see. The mistake was introduced in this commit:
> 
> commit 90fddabf5818367c6bd1fe1b256a10e01827862f
> Author: David Howells <dhowells@...hat.com>
> Date:   Wed Mar 24 09:43:00 2010 +0000
> 
>     Document Linux's circular buffering capabilities
> 
>     Document the circular buffering capabilities available in Linux.
> 
>     Signed-off-by: David Howells <dhowells@...hat.com>
>     Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
>     Reviewed-by: Randy Dunlap <rdunlap@...otime.net>
>     Reviewed-by: Stefan Richter <stefanr@...6.in-berlin.de>
>     Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>

Please understand that I am not arguing for no change, and that I do
appreciate your attention to detail and your willingness to propose an
actual change.

In this sentence in the original:  "The producer will look something like
this", the words "something like" are important, as in the following is
pseudocode rather than code that can be built.  You appear to be looking
to make this be actual code, which would be a good thing.  Except that
we have this:

	struct circ_buf {
		char *buf;
		int head;
		int tail;
	};

As you can see, there is no ->vals member, which is likely to look silly
to some future reader, just as the conflict between "buffer->size"
on the one hand and "buffer[head]" on the other looked silly to you,
and rightly so.  And both you and that potential future reader would be
quite justified in their judging the pseudo code as being silly.

So if we are going to change this, why not bite the bullet and make it
be real code that lives within the confines of the circ_buf structure?
Or, alternatively, that creates its own structure on the other?
Either approach will be a larger change, but the result will be more
helpful to a larger fraction of the future readers.

One approach would be to look for uses of the circ_buf structure,
CIRC_SPACE(), and CIRC_CNT() in the kernel and create an example
based on a simple use case.

Would you be willing to take this on?

							Thanx, Paul

> > > > ---
> > > >   Documentation/core-api/circular-buffers.rst | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/Documentation/core-api/circular-buffers.rst b/Documentation/core-api/circular-buffers.rst
> > > > index 50966f66e398..b697915a2bd0 100644
> > > > --- a/Documentation/core-api/circular-buffers.rst
> > > > +++ b/Documentation/core-api/circular-buffers.rst
> > > > @@ -161,7 +161,7 @@ The producer will look something like this::
> > > >   	if (CIRC_SPACE(head, tail, buffer->size) >= 1) {
> > > >   		/* insert one item into the buffer */
> > > > -		struct item *item = buffer[head];
> > > > +		struct item *item = buffer->vals[head];
> > > >   		produce_item(item);
> > > > @@ -203,7 +203,7 @@ The consumer will look something like this::
> > > >   	if (CIRC_CNT(head, tail, buffer->size) >= 1) {
> > > >   		/* extract one item from the buffer */
> > > > -		struct item *item = buffer[tail];
> > > > +		struct item *item = buffer->vals[tail];
> > > >   		consume_item(item);
> > > > -- 
> > > > 2.43.0
> > >          Thanks, Akira
> > > 
> 
> Thanks,
> 
> Carlos
>