[PATCH 0/8] membuff: Add tests and update to support a flag for empty/full

[Simon Glass]

Hi Rasmus,

On Thu, 6 Mar 2025 at 07:21, Rasmus Villemoes <ravi at prevas.dk> wrote:
>
> On Wed, Mar 05 2025, Simon Glass <sjg at chromium.org> wrote:
>
> > Hi Rasmus,
> >
> > On Tue, 4 Mar 2025 at 11:55, Rasmus Villemoes <ravi at prevas.dk> wrote:
> >>
> >> On Tue, Mar 04 2025, Simon Glass <sjg at chromium.org> wrote:
> >>
> >> > Hi Rasmus,
> >> >
> >> > On Fri, 18 Oct 2024 at 08:55, Simon Glass <sjg at chromium.org> wrote:
> >> >>
> >> >> Hi Rasmus,
> >> >>
> >> >> On Fri, 18 Oct 2024 at 01:05, Rasmus Villemoes <ravi at prevas.dk> wrote:
> >> >> >
> >> >> >
> >> >> > If you want to do the churn of renaming anyway, I suggest doing it by
> >> >> > adding an implementation using the proper scheme under the new name,
> >> >> > switch users over, and dropping the old. IMO, this series as-is brings
> >> >> > no value (except for the tests, of course).
> >> >>
> >> >> OK. Do you think this series gets us closer to that, or further away?
> >> >
> >> > I didn't get a response to this (which is not a problem, I miss things
> >> > all the time). Anyway I don't like the power-of-two restriction and
> >> > you can see my other responses above. I've applied this to my tree as
> >> > I want to have the tests in place.
> >>
> >> I stand by my earlier comments that this is the wrong way to implement a
> >> circular buffer. I hope Tom doesn't pull this.
> >
> > OK. Are you saying that you think it should only support power-of-two
> > sizes,
>
> Yes, because that's the natural way to implement such a simple data
> structure on real hardware, and I don't agree that it limits its
> usefulness in any way. See how the linux kernel implements unix pipes,
> and their kfifo helper.
>
> > or something else? What specifically do you want?
>
> I don't "want" anything in particular. I'm merely voicing my opinion
> that I consider this approach to implementing a circular buffer
> inferior.
>
> > This series:
> > - adds tests
>
> Yes, always a good thing.
>
> > - renames to membuf
>
> While we can agree that is better than with two f's, I don't see it as a
> huge virtue.
>
> > - shows how we could switch to using an empty/full flag instead of
> > leaving an empty slot, so we can see the code-size image
>
> As I've said previously, I believe that actually makes the whole thing
> even worse. Not using the natural wrapping/masking is already
> error-prone enough, but needing to have two parallel implementations
> living in the same source files with #ifdeffery; please, no. Do one or
> the other, unconditionally.

Sure, I can remove that, since it seems that the flag is not that
useful. I'll send a patch.

>
> > - does all this without requiring the size to be a power of two (which
> > limits its usefulness IMO)
>
> On that, we'll just have to agree to disagree.

An example of this is parsing a text file. At present you can just:

membuf_init(size...)
membuf_readline()...

whereas presumably with power-of-two you would need to

int newsize = ALIGN(size, next power of two....)

void *newptr = malloc(newsize)
memcpy(newptr, data, size)
membuf_init(newsize...)
membuf_setup([so that only 'size' bytes are present])
membuf_readline()...

>
> Rasmus
>
> PS: Something like
> https://gist.github.com/Villemoes/332ac7a5dfc983c58ad40773c7bc6385 is
> what I consider a simple and readable implementation (it may be buggy,
> of course).

There are no tests, so who knows? But also, it doesn't allow 'raw'
access which is considerably more efficient in many situations, e.g.
you can find out the amount of space, read directly into that space
without a separate buffer, then confirm how much you read.

Certainly my code can be improved.

>
> Frankly, the triple-pointer use in membuff.c frightens me, and that the
> implementation of something as simple as membuff_avail() involves a
> struct copy and calling membuff_getraw() in a loop makes it really hard
> to reason about and verify.

We have tests for verification. But how about you send a patch against
the API in my tree? If we can come up with something we both like,
then perhaps Tom might apply it.

Regards,
Simon