libbpf: ringbuf: allow to partially consume items

Commit Message

Andrea Righi March 10, 2024, 3:47 p.m. UTC

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 <[e-mail]>
---
 tools/lib/bpf/ringbuf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Andrii Nakryiko March 11, 2024, 5:55 p.m. UTC | #1

On Sun, Mar 10, 2024 at 8:47 AM Andrea Righi <[e-mail]> 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 <[e-mail]>
> ---
>  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.

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.

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

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

Andrea Righi March 11, 2024, 8:25 p.m. UTC | #2

On Mon, Mar 11, 2024 at 10:55:57AM -0700, Andrii Nakryiko wrote:
> On Sun, Mar 10, 2024 at 8:47 AM Andrea Righi <[e-mail]> 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 <[e-mail]>
> > ---
> >  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
> >
> >

Andrii Nakryiko March 12, 2024, 10:42 p.m. UTC | #3

On Mon, Mar 11, 2024 at 1:25 PM Andrea Righi <[e-mail]> wrote:
>
> On Mon, Mar 11, 2024 at 10:55:57AM -0700, Andrii Nakryiko wrote:
> > On Sun, Mar 10, 2024 at 8:47 AM Andrea Righi <[e-mail]> 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 <[e-mail]>
> > > ---
> > >  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
>

cool, great that you at least have a work-around


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

yes, that's what the contract is right now

> Why any user callback would return > 0 then?

this is not the code I can control and ringbuf API was like that for a
long time, which is why I'm saying I'm hesitant to make these changes

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

Yes, definitely a new API, but that's not a big problem. Though I'm
wondering if ring_buffer__consume_one() would be a more flexible API,
where user would have more flexible control. Either way libbpf is
doing smp_store_release() after each consumed element, so I don't
think it will have any downsides in terms of performance.

So please consider contributing a patch for this new API.

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

Patch

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) {
+					/* 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);