Design proposal for new ring buffer API #3848
Conversation
mikeagun
requested review from
dthaler,
poornagmsft,
Alan-Jowett,
saxena-anurag,
shankarseal,
dv-msft,
gtrevi,
shpalani,
matthewige,
mtfriesen and
rectified95
as code owners
mikeagun
changed the title
|
Converting to draft while I make a few design updates. |
Splits the design so callback consumers use the existing libbpf APIs, and mapped memory consumers use the new windows-specific functions.
|
|
||
| If the `RINGBUF_FLAG_NO_AUTO_CALLBACK` flag is set, callbacks will not automatically be called and `ring_buffer__poll()` | ||
| should be called to poll for available data and invoke the callback. On Windows a timeout of zero can be passed to | ||
| `ring_buffer__poll()` to get the same behaviour as `ring_buffer__consume()`. |
There was a problem hiding this comment.
What is the proposed behavior if ring_buffer__consume() is called when the RINGBUF_FLAG_NO_AUTO_CALLBACK flag is set?
| ## Overview | ||
|
|
||
| The current ringbuffer uses a pure callback-based approach to reading the ringbuffer. | ||
| Linux also supports memory-mapped polling consumers, which can't be directly supported in the current model. |
There was a problem hiding this comment.
What happens if you try to port code to do so? Would it fail to compile, compile and crash when run, compile and have some other behavior when run, or what?
| On Windows asynchronous events are supported by default, | ||
| so nothing extra needs to be done for callbacks to be invoked. | ||
|
|
||
| If the `RINGBUF_FLAG_NO_AUTO_CALLBACK` flag is set, callbacks will not automatically be called and `ring_buffer__poll()` |
There was a problem hiding this comment.
Why should the default be different between Windows vs Linux?
Why not a RINGBUF_FLAG_AUTO_CALLBACK flag instead?
| * - 0 (auto/default): Notify if consumer has caught up. | ||
| * - BPF_RB_FORCE_WAKEUP - Always notify consumer. |
There was a problem hiding this comment.
nit: Inconsistent use of punctuation (: vs -)
| * | ||
| */ | ||
| ebpf_result_t | ||
| ebpf_ring_buffer_output(ebpf_ring_buffer_t* ring, uint8_t* data, size_t length, size_t flags) |
There was a problem hiding this comment.
Please add SAL annotations to ebpf_* APIs in this doc.
| size_t sz; /* size of this struct, for forward/backward compatiblity */ | ||
| uint64_t flags; /* ring buffer option flags */ |
There was a problem hiding this comment.
Fix indentation so these lines match
| * @param[out] producer pointer* to start of read-only mapped producer pages | ||
| * @param[out] consumer pointer* to start of read-write mapped consumer page | ||
| * | ||
| * @returns EBPF_SUCCESS on success, or error |
There was a problem hiding this comment.
Use retval rather than returns, e.g.
| * @returns EBPF_SUCCESS on success, or error | |
| * @retval EBPF_SUCCESS The operation was successful. | |
| * @retval other An error occurred. |
| ebpf_result_t ebpf_ring_buffer_get_buffer(fd_t map_fd, void **producer, void **consumer); | ||
|
|
||
| /** | ||
| * get the wait handle to use with WaitForSingleObject/WaitForMultipleObject |
There was a problem hiding this comment.
| * get the wait handle to use with WaitForSingleObject/WaitForMultipleObject | |
| * Get the wait handle to use with WaitForSingleObject/WaitForMultipleObject. |
| * | ||
| * @returns Pointer to consumer offset | ||
| */ | ||
| uint64_t* rb__consumer_offset(void *cons); |
There was a problem hiding this comment.
Does this exact same prototype exist on Linux? I couldn't find it, I could only find
https://github.com/torvalds/linux/blob/master/include/uapi/linux/bpf.h#L4558 as a helper function.
This doesn't follow https://github.com/microsoft/ebpf-for-windows/blob/main/docs/DevelopmentGuide.md#coding-conventions so is only ok if it's identical to Linux.
| * | ||
| * @returns Pointer to producer offset | ||
| */ | ||
| volatile const uint64_t* rb__producer_offset(volatile const void *prod); |
There was a problem hiding this comment.
Does this exact same prototype exist on Linux? I couldn't find it, I could only find
https://github.com/torvalds/linux/blob/master/include/uapi/linux/bpf.h#L4558
| // consumer code | ||
| struct ring_buffer_opts opts; | ||
| opts.sz = sizeof(opts); | ||
| opts.flags = RINGBUF_FLAG_NO_AUTO_CALLBACK; //no automatic callbacks |
There was a problem hiding this comment.
One should not need to opt into Linux behavior, it should be the default.
We want to avoid gratuitous differences.
Description
Add docs/RingBuffer.md with proposal for new ebpf ring buffer map API exposing mapped memory so consumers can directly read the records written by the producer (similar to linux mmap/epoll style ring buffer consumer).
Testing
N/A
Documentation
Documentation only.
Installation
N/A