Re: [PATCH] libbpf: ringbuf: allow to partially consume items

[Andrea Righi <andrea.righi@canonical.com>]

On Mon, Mar 11, 2024 at 10:55:57AM -0700, Andrii Nakryiko wrote:
> On Sun, Mar 10, 2024 at 8:47 AM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > Instead of always consuming all items from a ring buffer in a greedy
> > way, allow to stop when the callback returns a value > 0.
> >
> > This allows to distinguish between an error condition and an intentional
> > stop condition by returning a non-negative non-zero value from the ring
> > buffer callback.
> >
> > This can be useful, for example, to consume just a single item from the
> > ring buffer.
> >
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> >  tools/lib/bpf/ringbuf.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> > index aacb64278a01..dd8908eb3204 100644
> > --- a/tools/lib/bpf/ringbuf.c
> > +++ b/tools/lib/bpf/ringbuf.c
> > @@ -265,6 +265,14 @@ static int64_t ringbuf_process_ring(struct ring *r)
> >                                         return err;
> >                                 }
> >                                 cnt++;
> > +                               if (err > 0) {
> 
> So libbpf already stops at any err < 0 (and sets correct consumer
> pos). So you could already get desired behavior by just returning your
> own error code. If you need count, you'd have to count it yourself
> through custom context, that's a bit of inconvenience.

Yep, that's exactly what I'm doing right now.

To give more context, here's the code:
https://github.com/sched-ext/scx/blob/b7c06b9ed9f72cad83c31e39e9c4e2cfd8683a55/rust/scx_rustland_core/src/bpf.rs#L217

> 
> But on the other hand, currently if user callback returns anything > 0
> they keep going and that return value is ignored. Your change will
> break any such user pretty badly. So I'm a bit hesitant to do this.

So, returning a value > 0 should have the same behavior as returning 0?
Why any user callback would return > 0 then?

> 
> Is there any reason you can't just return error code (libbpf doesn't
> do anything with it, just passes it back, so it might as well be
> `-cnt`, if you need that).

Sure, I can keep using my special error code to stop. It won't be a
problem for my particular use case.

Actually, one thing that it would be nice to have is a way to consume up
to a certain amount of items, let's say I need to copy multiple items
from the ring buffer to a limited user buffer. But that would require a
new API I guess, in order to pass the max counter... right?

Thanks,
-Andrea

> 
> pw-bot: cr
> 
> > +                                       /* update consumer pos and return the
> > +                                        * total amount of items consumed.
> > +                                        */
> > +                                       smp_store_release(r->consumer_pos,
> > +                                                         cons_pos);
> > +                                       goto done;
> > +                               }
> >                         }
> >
> >                         smp_store_release(r->consumer_pos, cons_pos);
> > --
> > 2.43.0
> >
> >