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.
