[PATCH bpf-next 0/2] add ring_buffer__query

[PATCH bpf-next 0/2] add ring_buffer__query

[Martin Kelly]

Add a ring_buffer__query function mimicking the existing bpf_ringbuf_query
helper but accessible from usermode. This can be useful for programs to query
ringbuffer information at runtime, e.g. seeing how much free space the
ringbuffers currently have.

Martin Kelly (2):
  libbpf: add ring_buffer__query
  selftests/bpf: add tests for ring_buffer__query

 tools/lib/bpf/libbpf.h                        |  2 ++
 tools/lib/bpf/libbpf.map                      |  1 +
 tools/lib/bpf/ringbuf.c                       | 33 +++++++++++++++++++
 .../selftests/bpf/prog_tests/ringbuf.c        | 22 +++++++++++++
 4 files changed, 58 insertions(+)

-- 
2.34.1

[PATCH bpf-next 1/2] libbpf: add ring_buffer__query

[Martin Kelly]

Add ring_buffer__query to fetch ringbuffer information from userspace,
working the same as the bpf_ringbuf_query helper.

Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
---
 tools/lib/bpf/libbpf.h   |  2 ++
 tools/lib/bpf/libbpf.map |  1 +
 tools/lib/bpf/ringbuf.c  | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 0e52621cba43..4ceed3ffabc2 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -1248,6 +1248,8 @@ LIBBPF_API int ring_buffer__add(struct ring_buffer *rb, int map_fd,
 LIBBPF_API int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms);
 LIBBPF_API int ring_buffer__consume(struct ring_buffer *rb);
 LIBBPF_API int ring_buffer__epoll_fd(const struct ring_buffer *rb);
+LIBBPF_API __u64 ring_buffer__query(struct ring_buffer *rb, unsigned int index,
+				    __u64 flags);
 
 struct user_ring_buffer_opts {
 	size_t sz; /* size of this struct, for forward/backward compatibility */
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 57712321490f..cbb3dc39446e 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -400,4 +400,5 @@ LIBBPF_1.3.0 {
 		bpf_program__attach_netfilter;
 		bpf_program__attach_tcx;
 		bpf_program__attach_uprobe_multi;
+		ring_buffer__query;
 } LIBBPF_1.2.0;
diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
index 02199364db13..85ccac7a2db3 100644
--- a/tools/lib/bpf/ringbuf.c
+++ b/tools/lib/bpf/ringbuf.c
@@ -69,6 +69,15 @@ static void ringbuf_unmap_ring(struct ring_buffer *rb, struct ring *r)
 	}
 }
 
+static unsigned long ringbuf_avail_data_sz(struct ring *r)
+{
+	unsigned long cons_pos, prod_pos;
+
+	cons_pos = smp_load_acquire(r->consumer_pos);
+	prod_pos = smp_load_acquire(r->producer_pos);
+	return prod_pos - cons_pos;
+}
+
 /* Add extra RINGBUF maps to this ring buffer manager */
 int ring_buffer__add(struct ring_buffer *rb, int map_fd,
 		     ring_buffer_sample_fn sample_cb, void *ctx)
@@ -323,6 +332,30 @@ int ring_buffer__epoll_fd(const struct ring_buffer *rb)
 	return rb->epoll_fd;
 }
 
+/* A userspace analogue to bpf_ringbuf_query for a particular ringbuffer index
+ * managed by this ringbuffer manager. Flags has the same arguments as
+ * bpf_ringbuf_query, and the index given is a 0-based index tracking the order
+ * the ringbuffers were added via ring_buffer__add. Returns the data requested
+ * according to flags.
+ */
+__u64 ring_buffer__query(struct ring_buffer *rb, unsigned int index, __u64 flags)
+{
+	struct ring *ring = &rb->rings[index];
+
+	switch (flags) {
+	case BPF_RB_AVAIL_DATA:
+		return ringbuf_avail_data_sz(ring);
+	case BPF_RB_RING_SIZE:
+		return ring->mask + 1;
+	case BPF_RB_CONS_POS:
+		return smp_load_acquire(ring->consumer_pos);
+	case BPF_RB_PROD_POS:
+		return smp_load_acquire(ring->producer_pos);
+	default:
+		return 0;
+	}
+}
+
 static void user_ringbuf_unmap_ring(struct user_ring_buffer *rb)
 {
 	if (rb->consumer_pos) {
-- 
2.34.1

[PATCH bpf-next 2/2] selftests/bpf: add tests for ring_buffer__query

[Martin Kelly]

Confirm we get the same results using ring_buffer__query from the
usermode side as from the corresponding BPF helper.

Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
---
 .../selftests/bpf/prog_tests/ringbuf.c        | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
index ac104dc652e3..93dcb92fc0c5 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
@@ -91,6 +91,7 @@ static void ringbuf_subtest(void)
 	int err, cnt, rb_fd;
 	int page_size = getpagesize();
 	void *mmap_ptr, *tmp_ptr;
+	__u64 val;
 
 	skel = test_ringbuf_lskel__open();
 	if (CHECK(!skel, "skel_open", "skeleton open failed\n"))
@@ -176,6 +177,27 @@ static void ringbuf_subtest(void)
 	      "err_prod_pos", "exp %ld, got %ld\n",
 	      3L * rec_sz, skel->bss->prod_pos);
 
+	/* verify the same results from the usermode side */
+	val = ring_buffer__query(ringbuf, 0, BPF_RB_AVAIL_DATA);
+	CHECK(val != 3 * rec_sz,
+	      "err_query_avail_size", "exp %ld, got %llu\n",
+	      3L * rec_sz, val);
+
+	val = ring_buffer__query(ringbuf, 0, BPF_RB_RING_SIZE);
+	CHECK(val != page_size,
+	      "err_query_ring_size", "exp %ld, got %llu\n",
+	      (long)page_size, val);
+
+	val = ring_buffer__query(ringbuf, 0, BPF_RB_CONS_POS);
+	CHECK(val != 0,
+	      "err_query_cons_pos", "exp %ld, got %llu\n",
+	      0L, val);
+
+	val = ring_buffer__query(ringbuf, 0, BPF_RB_PROD_POS);
+	CHECK(val != 3 * rec_sz,
+	      "err_query_prod_pos", "exp %ld, got %llu\n",
+	      3L * rec_sz, val);
+
 	/* poll for samples */
 	err = ring_buffer__poll(ringbuf, -1);
 
-- 
2.34.1

Re: [PATCH bpf-next 1/2] libbpf: add ring_buffer__query

[Andrii Nakryiko]

On Thu, Sep 7, 2023 at 4:42 PM Martin Kelly
<martin.kelly@crowdstrike.com> wrote:
>
> Add ring_buffer__query to fetch ringbuffer information from userspace,
> working the same as the bpf_ringbuf_query helper.
>
> Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
> ---
>  tools/lib/bpf/libbpf.h   |  2 ++
>  tools/lib/bpf/libbpf.map |  1 +
>  tools/lib/bpf/ringbuf.c  | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 0e52621cba43..4ceed3ffabc2 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -1248,6 +1248,8 @@ LIBBPF_API int ring_buffer__add(struct ring_buffer *rb, int map_fd,
>  LIBBPF_API int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms);
>  LIBBPF_API int ring_buffer__consume(struct ring_buffer *rb);
>  LIBBPF_API int ring_buffer__epoll_fd(const struct ring_buffer *rb);
> +LIBBPF_API __u64 ring_buffer__query(struct ring_buffer *rb, unsigned int index,
> +                                   __u64 flags);
>
>  struct user_ring_buffer_opts {
>         size_t sz; /* size of this struct, for forward/backward compatibility */
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 57712321490f..cbb3dc39446e 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -400,4 +400,5 @@ LIBBPF_1.3.0 {
>                 bpf_program__attach_netfilter;
>                 bpf_program__attach_tcx;
>                 bpf_program__attach_uprobe_multi;
> +               ring_buffer__query;
>  } LIBBPF_1.2.0;
> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index 02199364db13..85ccac7a2db3 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
> @@ -69,6 +69,15 @@ static void ringbuf_unmap_ring(struct ring_buffer *rb, struct ring *r)
>         }
>  }
>
> +static unsigned long ringbuf_avail_data_sz(struct ring *r)
> +{
> +       unsigned long cons_pos, prod_pos;
> +
> +       cons_pos = smp_load_acquire(r->consumer_pos);
> +       prod_pos = smp_load_acquire(r->producer_pos);
> +       return prod_pos - cons_pos;
> +}
> +
>  /* Add extra RINGBUF maps to this ring buffer manager */
>  int ring_buffer__add(struct ring_buffer *rb, int map_fd,
>                      ring_buffer_sample_fn sample_cb, void *ctx)
> @@ -323,6 +332,30 @@ int ring_buffer__epoll_fd(const struct ring_buffer *rb)
>         return rb->epoll_fd;
>  }
>
> +/* A userspace analogue to bpf_ringbuf_query for a particular ringbuffer index
> + * managed by this ringbuffer manager. Flags has the same arguments as
> + * bpf_ringbuf_query, and the index given is a 0-based index tracking the order
> + * the ringbuffers were added via ring_buffer__add. Returns the data requested
> + * according to flags.
> + */
> +__u64 ring_buffer__query(struct ring_buffer *rb, unsigned int index, __u64 flags)

I can see how this might be useful, but I don't think this exact API
and approach will work well.

First, I'd just add getters to get consumer and producer position and
producer. User-space code can easily derive available data from that
(and it's always racy and best effort to determine amount of data
enqueued in ringbuf, so having this as part of user-space API also
seems a bit off). RING_SIZE is something that user-space generally
should know already, or it can get it through a bpf_map__max_entries()
helper.

Second, this `unsigned int index` is not a good interface. There is
nothing in ring_buffer APIs that operates based on such an index.

So we need to think a bit more about the better way to provide this in
libbpf UAPI, IMO.

> +{
> +       struct ring *ring = &rb->rings[index];
> +
> +       switch (flags) {
> +       case BPF_RB_AVAIL_DATA:
> +               return ringbuf_avail_data_sz(ring);
> +       case BPF_RB_RING_SIZE:
> +               return ring->mask + 1;
> +       case BPF_RB_CONS_POS:
> +               return smp_load_acquire(ring->consumer_pos);
> +       case BPF_RB_PROD_POS:
> +               return smp_load_acquire(ring->producer_pos);
> +       default:
> +               return 0;
> +       }
> +}
> +
>  static void user_ringbuf_unmap_ring(struct user_ring_buffer *rb)
>  {
>         if (rb->consumer_pos) {
> --
> 2.34.1
>

Re: Re: [PATCH bpf-next 1/2] libbpf: add ring_buffer__query

[Martin Kelly]

On 9/8/23 17:25, Andrii Nakryiko wrote:
> On Thu, Sep 7, 2023 at 4:42 PM Martin Kelly
> <martin.kelly@crowdstrike.com> wrote:
>> +/* A userspace analogue to bpf_ringbuf_query for a particular ringbuffer index
>> + * managed by this ringbuffer manager. Flags has the same arguments as
>> + * bpf_ringbuf_query, and the index given is a 0-based index tracking the order
>> + * the ringbuffers were added via ring_buffer__add. Returns the data requested
>> + * according to flags.
>> + */
>> +__u64 ring_buffer__query(struct ring_buffer *rb, unsigned int index, __u64 flags)
> I can see how this might be useful, but I don't think this exact API
> and approach will work well.
>
> First, I'd just add getters to get consumer and producer position and
> producer. User-space code can easily derive available data from that
> (and it's always racy and best effort to determine amount of data
> enqueued in ringbuf, so having this as part of user-space API also
> seems a bit off). RING_SIZE is something that user-space generally
> should know already, or it can get it through a bpf_map__max_entries()
> helper.

I agree that getting available data is naturally racy, though there's no 
avoiding that. Since producer and consumer are both read atomically, and 
since producer >= consumer absent kernel bugs, I don't see this causing 
any harm though, as long as we document it in the API. Despite being 
technically racy, it's very useful to know the rough levels of the 
ringbuffers for monitoring things like "the ringbuffer is close to 
full", which usermode may want to take some action on (e.g. alerting or 
lowering log level to add backpressure). Sure, you could do it by having 
BPF populate the levels using bpf_ringbuf_query and a map, but if you're 
just polling on this from usermode, being able to check this directly is 
more efficient and leads to simpler code. For instance, if you do this 
from BPF and have many ringbuffers via a map-in-map type, you end up 
having to create two separate map-in-maps, which uses more memory and is 
yet another map for usermode to manage just to get this info.

If you prefer to simply expose producer and consumer and not available 
data method though, that's ok with me. That said, it means code using 
this directly would break in the future if somehow the implementation 
changed; if we expose available data directly, we can change the 
implementation in that case and avoid the concern.

I have no issue dropping RING_SIZE; I included it simply to mirror 
ringbuf_query, but if we're no longer mirroring it, then we don't need 
to keep it.

> Second, this `unsigned int index` is not a good interface. There is
> nothing in ring_buffer APIs that operates based on such an index.
The index is the heart of the problem, and I expected there might be 
concerns about it. The issue is that right now, only struct ring_buffer 
is exposed to the user, but this struct actually contains many 
ringbuffers, with no API for referencing any particular ringbuffer.

One thing we could do is expose struct ring * as an opaque type:

struct ring *ring_buffer__get(struct ring_buffer *rb, unsigned int index);
__u32 ring_buffer__count(struct ring_buffer *rb);

Then individual ringbuffers could have methods like:

__u64 ring__get_avail_data(struct ring *r);
__u64 ring__get_producer_pos(struct ring *r);
__u64 ring__get_consumer_pos(struct ring *r);

And usermode could do something like this:

for (i = 0; i < ring_buffer__count(rb); i++) {
     struct ring *r = ring_buffer__get(rb);
     avail_data = ring__get_avail_data(r);
     /* do some logic with avail_data */
}

Because struct ring_buffer currently contains many ringbuffers, I think 
we need to add an index, either directly in these methods or by exposing 
struct ring * as an opaque type.

I appreciate your response; please let me know what you think.

Re: Re: [PATCH bpf-next 1/2] libbpf: add ring_buffer__query

[Andrii Nakryiko]

On Mon, Sep 11, 2023 at 9:51 AM Martin Kelly
<martin.kelly@crowdstrike.com> wrote:
>
> On 9/8/23 17:25, Andrii Nakryiko wrote:
> > On Thu, Sep 7, 2023 at 4:42 PM Martin Kelly
> > <martin.kelly@crowdstrike.com> wrote:
> >> +/* A userspace analogue to bpf_ringbuf_query for a particular ringbuffer index
> >> + * managed by this ringbuffer manager. Flags has the same arguments as
> >> + * bpf_ringbuf_query, and the index given is a 0-based index tracking the order
> >> + * the ringbuffers were added via ring_buffer__add. Returns the data requested
> >> + * according to flags.
> >> + */
> >> +__u64 ring_buffer__query(struct ring_buffer *rb, unsigned int index, __u64 flags)
> > I can see how this might be useful, but I don't think this exact API
> > and approach will work well.
> >
> > First, I'd just add getters to get consumer and producer position and
> > producer. User-space code can easily derive available data from that
> > (and it's always racy and best effort to determine amount of data
> > enqueued in ringbuf, so having this as part of user-space API also
> > seems a bit off). RING_SIZE is something that user-space generally
> > should know already, or it can get it through a bpf_map__max_entries()
> > helper.
>
> I agree that getting available data is naturally racy, though there's no
> avoiding that. Since producer and consumer are both read atomically, and
> since producer >= consumer absent kernel bugs, I don't see this causing
> any harm though, as long as we document it in the API. Despite being
> technically racy, it's very useful to know the rough levels of the
> ringbuffers for monitoring things like "the ringbuffer is close to
> full", which usermode may want to take some action on (e.g. alerting or
> lowering log level to add backpressure). Sure, you could do it by having
> BPF populate the levels using bpf_ringbuf_query and a map, but if you're
> just polling on this from usermode, being able to check this directly is
> more efficient and leads to simpler code. For instance, if you do this
> from BPF and have many ringbuffers via a map-in-map type, you end up
> having to create two separate map-in-maps, which uses more memory and is
> yet another map for usermode to manage just to get this info.

Yes, I see the utility. You don't have to argue why this is useful.

>
> If you prefer to simply expose producer and consumer and not available
> data method though, that's ok with me. That said, it means code using
> this directly would break in the future if somehow the implementation
> changed; if we expose available data directly, we can change the
> implementation in that case and avoid the concern.

There is no way that implementation will change to the point where
there will be no producer/consumer position or their meaning will
differ. This is set in stone as far as kernel API is concerned, so I
wouldn't worry about that.


>
> I have no issue dropping RING_SIZE; I included it simply to mirror
> ringbuf_query, but if we're no longer mirroring it, then we don't need
> to keep it.
>
> > Second, this `unsigned int index` is not a good interface. There is
> > nothing in ring_buffer APIs that operates based on such an index.
> The index is the heart of the problem, and I expected there might be
> concerns about it. The issue is that right now, only struct ring_buffer
> is exposed to the user, but this struct actually contains many
> ringbuffers, with no API for referencing any particular ringbuffer.
>
> One thing we could do is expose struct ring * as an opaque type:
>
> struct ring *ring_buffer__get(struct ring_buffer *rb, unsigned int index);
> __u32 ring_buffer__count(struct ring_buffer *rb);
>
> Then individual ringbuffers could have methods like:
>
> __u64 ring__get_avail_data(struct ring *r);
> __u64 ring__get_producer_pos(struct ring *r);
> __u64 ring__get_consumer_pos(struct ring *r);
>
> And usermode could do something like this:
>
> for (i = 0; i < ring_buffer__count(rb); i++) {
>      struct ring *r = ring_buffer__get(rb);
>      avail_data = ring__get_avail_data(r);
>      /* do some logic with avail_data */
> }
>
> Because struct ring_buffer currently contains many ringbuffers, I think
> we need to add an index, either directly in these methods or by exposing
> struct ring * as an opaque type.
>
> I appreciate your response; please let me know what you think.
>

I don't really have a better proposal (I tried). Let's go with
basically what you proposed, let's just follow libbpf naming
convention to not use "get" in getters. Something like this (notice
return types):

struct ring *ring_buffer__ring(struct ring_buffer *rb, int idx);

unsigned long ring__producer_pos(const struct ring *r);
unsigned long ring__consumer_pos(const struct ring *r);
/* document that this is racy estimate */
size_t ring__avail_data_size(const struct ring *r);
size_t ring__size(const struct ring *r);
int ring__map_fd(const struct ring *r);

so we have a more-or-less complete set of getters. We can probably
also later add consume() implementation (but not poll!).

Also, we'll need to decide whether returned struct ring * pointers are
invalidated on ring_buffer__add() or not. It probably would be less
error-prone if they were not, and it's easy to implement. Instead of
having `struct ring *rings` inside struct ring_buffer, we can have an
array of pointers and keep struct ring * pointers stable.

BTW, we should probably add producer/consumer pos for
user_ring_buffer, which is much more straightforward API-wise, because
it's always a single ringbuf, so no need to build extra abstractions.

Re: Re: Re: [PATCH bpf-next 1/2] libbpf: add ring_buffer__query

[Martin Kelly]

On 9/12/23 17:23, Andrii Nakryiko wrote:
>>> Second, this `unsigned int index` is not a good interface. There is
>>> nothing in ring_buffer APIs that operates based on such an index.
>> The index is the heart of the problem, and I expected there might be
>> concerns about it. The issue is that right now, only struct ring_buffer
>> is exposed to the user, but this struct actually contains many
>> ringbuffers, with no API for referencing any particular ringbuffer.
>>
>> One thing we could do is expose struct ring * as an opaque type:
>>
>> struct ring *ring_buffer__get(struct ring_buffer *rb, unsigned int index);
>> __u32 ring_buffer__count(struct ring_buffer *rb);
>>
>> Then individual ringbuffers could have methods like:
>>
>> __u64 ring__get_avail_data(struct ring *r);
>> __u64 ring__get_producer_pos(struct ring *r);
>> __u64 ring__get_consumer_pos(struct ring *r);
>>
>> And usermode could do something like this:
>>
>> for (i = 0; i < ring_buffer__count(rb); i++) {
>>       struct ring *r = ring_buffer__get(rb);
>>       avail_data = ring__get_avail_data(r);
>>       /* do some logic with avail_data */
>> }
>>
>> Because struct ring_buffer currently contains many ringbuffers, I think
>> we need to add an index, either directly in these methods or by exposing
>> struct ring * as an opaque type.
>>
>> I appreciate your response; please let me know what you think.
>>
> I don't really have a better proposal (I tried). Let's go with
> basically what you proposed, let's just follow libbpf naming
> convention to not use "get" in getters. Something like this (notice
> return types):
The below API looks good to me; I'll work on this.
>
> struct ring *ring_buffer__ring(struct ring_buffer *rb, int idx);
>
> unsigned long ring__producer_pos(const struct ring *r);
> unsigned long ring__consumer_pos(const struct ring *r);
> /* document that this is racy estimate */
> size_t ring__avail_data_size(const struct ring *r);
> size_t ring__size(const struct ring *r);
> int ring__map_fd(const struct ring *r);
>
> so we have a more-or-less complete set of getters. We can probably
> also later add consume() implementation (but not poll!).
>
> Also, we'll need to decide whether returned struct ring * pointers are
> invalidated on ring_buffer__add() or not. It probably would be less
> error-prone if they were not, and it's easy to implement. Instead of
> having `struct ring *rings` inside struct ring_buffer, we can have an
> array of pointers and keep struct ring * pointers stable.

I agree with that; this seems like a big foot-gun otherwise, especially 
as much of the time realloc will expand in-place and therefore nothing 
will break, so it could easily lead to a "rare crash" type of scenario 
for users.

>
> BTW, we should probably add producer/consumer pos for
> user_ring_buffer, which is much more straightforward API-wise, because
> it's always a single ringbuf, so no need to build extra abstractions.