-
Notifications
You must be signed in to change notification settings - Fork 245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor ebpf_ring_buffer to remove lock #4204
base: main
Are you sure you want to change the base?
Conversation
- eBPF ring buffer map has not been updated yet.
REQUIRE(remaining_failed_returns == 0); | ||
} | ||
|
||
TEST_CASE("ring_buffer_stress_tests", "[ring_buffer_stress]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where should the stress/speed tests go, or should they be in a new exe? We have ebpf_stress_tests, but that is closer to end-to-end testing while these test the internal ebpf_ring_buffer data structure.
If they stay here then they should probably be disabled by default -- there are already unit tests for ring buffer, these just try to hit any race conditions and take a while to complete.
(Also, there are print statements and comments in the stress tests that were useful during development but should be removed/reduced before merging -- depending on where these tests go they should be updated accordingly).
// - We only advance producer offset once the producer_offset matches the producer_reserve_offset we | ||
// originally got. | ||
// - This guarantees any records allocated before us are locked before we update offset. | ||
while (reserve_offset != ReadULong64Acquire(&ring->producer_offset)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments explaining why this is sufficient to ensure ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the comment above the spin loop to explain the ordering requirements and how they are fulfilled.
Remove lock from ebpf_ring_buffer and use acquire/release and atomics to ensure safe ordering.
Description
Removes the lock from ebpf_ring_buffer and uses acquire/release semantics (and one compare exchange) to ensure safe ordering in producer record reservation.
The ring buffer uses the same producer and consumer offsets, but now uses acquire/release and atomic operations to ensure safe ordering of updates to the offsets and record headers.
This change also updates the record header to match the 64 bit header used on linux.
There is no effect on the public API (the ring buffer record struct is changed but that isn't currently exposed by libbpf).
Producer Algorithm
ebpf_ring_buffer_reserve
This function ensures safe ordering of producers.
After the call to reserve, the producer can write to the record and then submit or discard.
The lock bit in the record header will be set when reserve returns, and will stay set until it is submitted or discarded so the consumer knows when it can read the record.
ebpf_ring_buffer_submit
ebpf_ring_buffer_discard
ebpf_ring_buffer_output
Consumer Algorithm
Currently ebpf-for-windows only exposes the libbpf callback-based consumer API, so there is no effect on consumers.
The new algorithm also supports lock-free polling consumers at the ebpf_ring_buffer level by taking advantage of the serialization of record header and producer offset updates done in ebpf_ring_buffer_reserve.
Testing
Updated existing ring buffer tests and added new stress tests to test the synchronization changes.
Documentation
Algorithm documented in ebpf_ring_buffer.c.
Installation
N/A