On Mon, Sep 11, 2023 at 9:51 AM Martin Kelly
<martin.kelly@xxxxxxxxxxxxxxx> wrote:
>
> On 9/8/23 17:25, Andrii Nakryiko wrote:
> > On Thu, Sep 7, 2023 at 4:42 PM Martin Kelly
> > <martin.kelly@xxxxxxxxxxxxxxx> 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.
