On Thu, Sep 7, 2023 at 4:42 PM Martin Kelly
<martin.kelly@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
> 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
>
