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

[Rasmus Villemoes <ravi@prevas.dk>]

On Wed, Mar 05 2025, Simon Glass <sjg@chromium.org> wrote:

> Hi Rasmus,
>
> On Tue, 4 Mar 2025 at 11:55, Rasmus Villemoes <ravi@prevas.dk> wrote:
>>
>> On Tue, Mar 04 2025, Simon Glass <sjg@chromium.org> wrote:
>>
>> > Hi Rasmus,
>> >
>> > On Fri, 18 Oct 2024 at 08:55, Simon Glass <sjg@chromium.org> wrote:
>> >>
>> >> Hi Rasmus,
>> >>
>> >> On Fri, 18 Oct 2024 at 01:05, Rasmus Villemoes <ravi@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.

> - 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.

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).

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.