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