aya,aya-ebpf: implement RingBuf #629

ajwerner · 2023-06-19T20:58:20Z

Note that this does not add any sort of special asynchronous support for reading from the ring buffer from userspace outside of what is offered by AsyncFd. Nevertheless, with this change, the RingBuf is broadly useful.

Closes: #12
Closes: #201

This change is

netlify · 2023-06-19T20:58:25Z

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`e2cf734`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/652709604895420008d0cd4f
😎 Deploy Preview	https://deploy-preview-629--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

aya/src/maps/mod.rs

aya/src/maps/ringbuf.rs

test/integration-test/Cargo.toml

test/integration-test/src/tests/ring_buf.rs

aya/src/maps/ringbuf.rs

bpf/aya-bpf/src/maps/ringbuf.rs

ajwerner

TFTR!

I renamed the files from ringbuf to ring_buf to match the convention elsewhere.

bpf/aya-bpf/src/maps/ringbuf.rs

test/integration-test/src/tests/ring_buf.rs

test/integration-test/Cargo.toml

aya/src/maps/mod.rs

aya/src/maps/ringbuf.rs

ajwerner · 2023-06-30T00:07:47Z

Okay, this is RFAL.

aya/src/maps/ring_buf.rs

bpf/aya-bpf/src/maps/ring_buf.rs

aya/src/programs/cgroup_skb.rs

ajwerner · 2023-06-30T18:20:49Z

@dave-tucker Thanks for the review! Adjusting the size when loading the probe makes sense.

ajwerner

Alright @dave-tucker, this is RFAL.

aya/src/maps/ring_buf.rs

aya/src/programs/cgroup_skb.rs

bpf/aya-bpf/src/maps/ring_buf.rs

ajwerner

@tamird I replaced the scheduler tracepoint tests with uprobes that share infrastructure with the other tests.The repro no longer occurred in the old formulation because the fetch_add did some additional synchronization. Now I keep track of the consumer position separately in the structure. It works out well enough. With that I was able to get a reliable repro when changing the ordering in a number of iterations that doesn't take too long even under software emulation.

Reviewable status: 6 of 33 files reviewed, all discussions resolved (waiting on @alessandrod and @tamird)

tamird

Reviewed 20 of 20 files at r62, 7 of 7 files at r63, 2 of 2 files at r64, 1 of 1 files at r65, 18 of 18 files at r66, 27 of 27 files at r67, 9 of 9 files at r68, 8 of 8 files at r69, 7 of 7 files at r70, 14 of 16 files at r71, 2 of 2 files at r72, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ajwerner and @alessandrod)

aya/src/maps/ring_buf.rs line 132 at r72 (raw file):

        let Self {
            data,
            consumer: ConsumerPos { pos, .. },

nit: destructure the metadata so we can see it only contains the mmap

aya/src/maps/ring_buf.rs line 172 at r72 (raw file):

        // Load the initial value of the consumer position. After this point we'll be the only
        // thread ever writing to this value, and we'll do it based on our local bookkeeping. We
        // just mmap'ed this data, so we can assume that the kernel has flushed the any previous

"the any"

aya/src/maps/ring_buf.rs line 173 at r72 (raw file):

        // thread ever writing to this value, and we'll do it based on our local bookkeeping. We
        // just mmap'ed this data, so we can assume that the kernel has flushed the any previous
        // consumers writes, so Relaxed would almost certainly work, but because this is

this is too many commas for one sentence.

aya/src/maps/ring_buf.rs line 277 at r72 (raw file):

        } = self;
        let pos = unsafe { mmap.ptr.cast().as_ref() };
        let data_pages = mmap.as_ref().get(*data_offset..).unwrap();

should we panic with words here?

aya/src/maps/ring_buf.rs line 324 at r72 (raw file):

            let header_ptr = data
                .get(offset..offset + core::mem::size_of::<AtomicU32>())
                .expect("offset out of bounds")

here and below should we panic with values?

test/integration-test/src/tests/ring_buf.rs line 1 at r72 (raw file):

use core::panic;

why core?

test/integration-test/src/tests/ring_buf.rs line 44 at r72 (raw file):

unsafe impl Pod for Registers {}

/// Generate a variable length vector of u64s.

what is this comment?

test/integration-test/src/tests/ring_buf.rs line 112 at r72 (raw file):

    let WithData(
        RingBufTest {
            ref mut ring_buf,

why are these ref? aren't you destructuring an owned value here?

same below

test/integration-test/src/tests/ring_buf.rs line 184 at r72 (raw file):

    let raw_fd = ring_buf.as_raw_fd();
    let async_fd = AsyncFd::new(raw_fd).unwrap();

this should hold the ring_buf rather than its fd

test/integration-test/src/tests/ring_buf.rs line 409 at r72 (raw file):

impl WriterThread {
    // This is enough messages for the test to fail relatively reliably in a hardware accelerated VM
    // with 2 vCPUs when the bug was present.

nit: "when the ring buffer implementation uses xxx rather than xxx" so the reader knows how to reverify the claim - it won't be in git.

tamird

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ajwerner and @alessandrod)

aya/src/maps/mod.rs line 270 at r69 (raw file):

    /// A [`HashMap`] map that uses a LRU eviction policy.
    LruHashMap(MapData),
    /// A [`PerCpuArray`] map

I added this commit, but if you wouldn't mind adding the missing periods in it, that'd be good.

ajwerner

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @alessandrod and @tamird)

aya/src/maps/mod.rs line 270 at r69 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I added this commit, but if you wouldn't mind adding the missing periods in it, that'd be good.

Done.

aya/src/maps/ring_buf.rs line 132 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: destructure the metadata so we can see it only contains the mmap

Done.

aya/src/maps/ring_buf.rs line 172 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

"the any"

Reworded.

aya/src/maps/ring_buf.rs line 173 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is too many commas for one sentence.

Done.

aya/src/maps/ring_buf.rs line 277 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

should we panic with words here?

Done.

aya/src/maps/ring_buf.rs line 324 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

here and below should we panic with values?

Done.

test/integration-test/src/tests/ring_buf.rs line 1 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why core?

Done.

test/integration-test/src/tests/ring_buf.rs line 44 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

what is this comment?

Done.

test/integration-test/src/tests/ring_buf.rs line 112 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why are these ref? aren't you destructuring an owned value here?

same below

Done.

test/integration-test/src/tests/ring_buf.rs line 184 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this should hold the ring_buf rather than its fd

Done.

test/integration-test/src/tests/ring_buf.rs line 409 at r72 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: "when the ring buffer implementation uses xxx rather than xxx" so the reader knows how to reverify the claim - it won't be in git.

Done.

tamird

Reviewed 23 of 23 files at r73, 7 of 7 files at r74, 16 of 16 files at r75, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ajwerner and @alessandrod)

aya/src/maps/ring_buf.rs line 57 at r75 (raw file):

}

impl<T: core::borrow::Borrow<MapData>> RingBuf<T> {

can you purge core? we're using std here.

test/integration-test/src/tests/ring_buf.rs line 25 at r75 (raw file):

}

impl core::ops::Add for Registers {

can you purge core? we're using std here.

ajwerner

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alessandrod and @tamird)

aya/src/maps/ring_buf.rs line 57 at r75 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can you purge core? we're using std here.

Done.

test/integration-test/src/tests/ring_buf.rs line 25 at r75 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can you purge core? we're using std here.

Done. Also imported some more things to remove some qualifications.

tamird

Reviewed 3 of 3 files at r76, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)

alessandrod

Thanks so much to everyone who's worked on this - the quality of the code, comments, readability etc in this PR is amazing.

Left a few minor comments.

aya/Cargo.toml

aya/src/maps/bloom_filter.rs

aya/src/maps/mod.rs

alessandrod · 2023-10-09T04:53:17Z

bpf/aya-bpf/src/maps/ring_buf.rs

+}
+
+impl RingBuf {
+    /// Declare a BPF ring buffer.


Nit: here and everywhere, use eBPF not BPF

eBPF is the name of the linux technology, BPF is the old thing and (unfortunately) the name of the
LLVM target. They're used interchangeably in the kernel but I think we should consitently use eBPF in
aya.

alessandrod · 2023-10-10T00:51:14Z

bpf/aya-bpf/src/maps/ring_buf.rs

+    /// Declare a BPF ring buffer.
+    ///
+    /// The linux kernel requires that `byte_size` be a power-of-2 multiple of the page size.
+    /// The loading program may coerce the size when loading the map.


There's an unnecessary newline after the period. Sentences follow each other. They're only separated by whitespaces.

Paragraphs are separated by two newlines.

alessandrod · 2023-10-11T00:40:24Z

test/integration-test/src/tests/ring_buf.rs

+
+        // Use the loader API to control the size of the ring_buf.
+        let mut bpf = BpfLoader::new()
+            .btf(Some(Btf::from_sys_fs().unwrap()).as_ref())


Is btf strictly required?

Not needed, not sure why this was here.

test/integration-ebpf/src/ring_buf.rs

alessandrod · 2023-10-11T01:06:46Z

test/integration-test/src/tests/ring_buf.rs

+        Self(RingBufTest::new(), {
+            let mut rng = rand::thread_rng();
+            // Generate more entries than there is space so we can test dropping entries.
+            let n = rng.gen_range(1..=RING_BUF_MAX_ENTRIES * 2);


The comment doesn't match the actual value of n. This sometimes will test
values over RING_BUF_MAX_ENTRIES. I don't think this kind of randomness here is
good? We should make sure both cases (in range, out of range) are always tested.

In general randomness in tests should be avoided. A test fails, I run it
again and I have new inputs, so it may or may not fail. In this case the random
values for the entries are probably fine, but imo n shouldn't be random here.

I parameterized the tests on N and then added test cases so that the data sizes for the relevant tests are both under and over capacity.

test/integration-test/src/tests/ring_buf.rs

tamird

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @ajwerner and @alessandrod)

aya/Cargo.toml line 23 at r76 (raw file):

Previously, alessandrod (Alessandro Decina) wrote…

Why this? default features include read which is a much larger superset of
read_core and compression, which we don't need.

The workspace Cargo.toml already says "default-features = false" so this is redundant.

Extracted into #812.

aya/src/maps/bloom_filter.rs line 85 at r76 (raw file):

Previously, alessandrod (Alessandro Decina) wrote…

Here and everywhere else, why import from libc and not std::ffi? We've talked
about making the libc dependency optional, this seems a change in the wrong direction

I didn't know that was a goal. Extracted the opposite change into #811.

aya/src/maps/mod.rs line 305 at r76 (raw file):

Previously, alessandrod (Alessandro Decina) wrote…

This sort of changes are good, but please next time consider splitting them in a
separate PR. This PR is already large and takes time to review, while sorting
variants is trivial and can be merged right away. It makes both PRs easier to
review and this one easier to rebase.

#813

mergify · 2023-10-11T15:29:06Z

@ajwerner, this pull request is now in conflict and requires a rebase.

mmap() is needed for the ring buffer implementation, so move it to a common module

tamird

Reviewed 25 of 25 files at r77, 9 of 9 files at r78, 16 of 16 files at r79, all commit messages.
Dismissed @alessandrod from 3 discussions.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ajwerner and @alessandrod)

ajwerner

TFTR!

ajwerner · 2023-10-11T14:06:43Z

bpf/aya-bpf/src/maps/ring_buf.rs

+    ///
+    /// Returns `None` if the ring buffer is full.
+    ///
+    /// Note: `T` must be aligned to no more than 8 bytes; it's not possible to fulfill larger


I've left handling larger alignments as an exercise for the reader; it's not clear it really comes up in practice.

The const_assert feature requires generic_const_exprs which is an incomplete feature. It makes me uneasy to opt into an incomplete feature. The hope is to make it always use const_assert once that feature is not marked as incomplete.

aya/src/maps/ring_buf.rs

ajwerner · 2023-10-11T19:16:51Z

bpf/aya-bpf/src/maps/ring_buf.rs

+}
+
+impl RingBuf {
+    /// Declare a BPF ring buffer.


ajwerner · 2023-10-11T19:16:57Z

bpf/aya-bpf/src/maps/ring_buf.rs

+    /// Declare a BPF ring buffer.
+    ///
+    /// The linux kernel requires that `byte_size` be a power-of-2 multiple of the page size.
+    /// The loading program may coerce the size when loading the map.


test/integration-ebpf/src/ring_buf.rs

ajwerner · 2023-10-11T19:51:32Z

test/integration-test/src/tests/ring_buf.rs

+
+        // Use the loader API to control the size of the ring_buf.
+        let mut bpf = BpfLoader::new()
+            .btf(Some(Btf::from_sys_fs().unwrap()).as_ref())


Not needed, not sure why this was here.

ajwerner · 2023-10-11T20:06:46Z

test/integration-test/src/tests/ring_buf.rs

+        Self(RingBufTest::new(), {
+            let mut rng = rand::thread_rng();
+            // Generate more entries than there is space so we can test dropping entries.
+            let n = rng.gen_range(1..=RING_BUF_MAX_ENTRIES * 2);


I parameterized the tests on N and then added test cases so that the data sizes for the relevant tests are both under and over capacity.

test/integration-test/src/tests/ring_buf.rs

This implements the userspace binding for RingBuf. Instead of streaming the samples as heap buffers, the process_ring function takes a callback to which we pass the event's byte region, roughly following [libbpf]'s API design. This avoids a copy and allows marking the consumer pointer in a timely manner. [libbpf]: https://github.com/libbpf/libbpf/blob/master/src/ringbuf.c Additionally, integration tests are added to demonstrate the usage of the new APIs and to ensure that they work end-to-end. Co-authored-by: William Findlay <william@williamfindlay.com> Co-authored-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>

tamird

Reviewed 3 of 3 files at r80, 4 of 4 files at r81, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @alessandrod)

alessandrod

This is great, thanks again everyone 🙏🎉

brevilo · 2023-10-12T07:56:15Z

This is awesome news since that feature was highly anticipated! 🙏

Hopefully it's going to be released soon such that everyone can integrate it easily.

Thanks everyone!

ajwerner changed the title ~~Ringbuf~~ aya,aya-ebpf: implement RingBuf Jun 19, 2023

ajwerner mentioned this pull request Jun 19, 2023

aya: Implement RingBuf #294

Closed

ajwerner force-pushed the ringbuf branch from 7d2c773 to b797e81 Compare June 19, 2023 21:00

ajwerner marked this pull request as ready for review June 21, 2023 14:00

tamird reviewed Jun 22, 2023

View reviewed changes

tamird requested a review from alessandrod June 22, 2023 15:34

marysaka reviewed Jun 28, 2023

View reviewed changes

bpf/aya-bpf/src/maps/ringbuf.rs Outdated Show resolved Hide resolved

ajwerner force-pushed the ringbuf branch from 4a1a381 to 2567976 Compare June 29, 2023 18:57

ajwerner commented Jun 29, 2023

View reviewed changes

ajwerner force-pushed the ringbuf branch 6 times, most recently from e701bec to 59dfdde Compare June 30, 2023 00:07

ajwerner force-pushed the ringbuf branch 2 times, most recently from 59ea0cb to 1cb308b Compare June 30, 2023 00:12

dave-tucker reviewed Jun 30, 2023

View reviewed changes

aya/src/maps/ring_buf.rs Outdated Show resolved Hide resolved

dave-tucker reviewed Jun 30, 2023

View reviewed changes

bpf/aya-bpf/src/maps/ring_buf.rs Outdated Show resolved Hide resolved

dave-tucker reviewed Jun 30, 2023

View reviewed changes

aya/src/programs/cgroup_skb.rs Outdated Show resolved Hide resolved

ajwerner force-pushed the ringbuf branch from 1cb308b to 974a8cd Compare July 1, 2023 19:28

ajwerner commented Jul 1, 2023

View reviewed changes

aya/src/maps/ring_buf.rs Outdated Show resolved Hide resolved

aya/src/programs/cgroup_skb.rs Outdated Show resolved Hide resolved

bpf/aya-bpf/src/maps/ring_buf.rs Outdated Show resolved Hide resolved

ajwerner force-pushed the ringbuf branch 2 times, most recently from ba53b67 to c843ef2 Compare July 2, 2023 05:45

tomaszjonak reviewed Jul 3, 2023

View reviewed changes

bpf/aya-bpf/src/maps/ring_buf.rs Show resolved Hide resolved

ajwerner force-pushed the ringbuf branch 2 times, most recently from 69551ef to 018a2bd Compare July 3, 2023 20:58

ajwerner commented Sep 28, 2023

View reviewed changes

ajwerner force-pushed the ringbuf branch 3 times, most recently from b3ebefb to 0e7d694 Compare September 28, 2023 14:32

tamird reviewed Sep 28, 2023

View reviewed changes

ajwerner force-pushed the ringbuf branch from 0e7d694 to d79b602 Compare September 28, 2023 16:28

ajwerner commented Sep 28, 2023

View reviewed changes

tamird approved these changes Sep 28, 2023

View reviewed changes

ajwerner force-pushed the ringbuf branch from d79b602 to b3a5c2b Compare September 28, 2023 17:27

ajwerner commented Sep 28, 2023

View reviewed changes

tamird approved these changes Sep 28, 2023

View reviewed changes

alessandrod reviewed Oct 11, 2023

View reviewed changes

tamird requested a review from alessandrod October 11, 2023 15:03

tamird approved these changes Oct 11, 2023

View reviewed changes

mergify bot added the needs-rebase label Oct 11, 2023

ajwerner and others added 2 commits October 11, 2023 12:21

aya: impl From<obj::InvalidMapTypeError> for MapTypeError

b73c0a4

aya: move mmap from perf_buffer.rs to sys/mod.rs

4af9d1b

mmap() is needed for the ring buffer implementation, so move it to a common module

tamird force-pushed the ringbuf branch from b3a5c2b to 33dae59 Compare October 11, 2023 16:25

mergify bot removed the needs-rebase label Oct 11, 2023

tamird reviewed Oct 11, 2023

View reviewed changes

ajwerner force-pushed the ringbuf branch from 33dae59 to 27354ca Compare October 11, 2023 20:26

ajwerner commented Oct 11, 2023

View reviewed changes

ajwerner force-pushed the ringbuf branch from 27354ca to e2cf734 Compare October 11, 2023 20:45

tamird approved these changes Oct 11, 2023

View reviewed changes

alessandrod approved these changes Oct 11, 2023

View reviewed changes

ajwerner merged commit 6284994 into aya-rs:main Oct 12, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aya,aya-ebpf: implement RingBuf #629

aya,aya-ebpf: implement RingBuf #629

ajwerner commented Jun 19, 2023 •

edited

netlify bot commented Jun 19, 2023 •

edited

ajwerner left a comment

ajwerner commented Jun 30, 2023

ajwerner commented Jun 30, 2023

ajwerner left a comment

ajwerner left a comment

tamird left a comment

tamird left a comment

ajwerner left a comment

tamird left a comment

ajwerner left a comment

tamird left a comment

alessandrod left a comment

alessandrod Oct 9, 2023

ajwerner Oct 11, 2023

alessandrod Oct 10, 2023

ajwerner Oct 11, 2023

alessandrod Oct 11, 2023

ajwerner Oct 11, 2023

alessandrod Oct 11, 2023

ajwerner Oct 11, 2023

tamird left a comment

mergify bot commented Oct 11, 2023

tamird left a comment

ajwerner left a comment

ajwerner Oct 11, 2023

ajwerner Oct 11, 2023

ajwerner Oct 11, 2023

ajwerner Oct 11, 2023

ajwerner Oct 11, 2023

tamird left a comment

alessandrod left a comment

brevilo commented Oct 12, 2023 •

edited

aya,aya-ebpf: implement RingBuf #629

aya,aya-ebpf: implement RingBuf #629

Conversation

ajwerner commented Jun 19, 2023 • edited

netlify bot commented Jun 19, 2023 • edited

✅ Deploy Preview for aya-rs-docs ready!

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner commented Jun 30, 2023

ajwerner commented Jun 30, 2023

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

alessandrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 11, 2023

tamird left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

alessandrod left a comment

Choose a reason for hiding this comment

brevilo commented Oct 12, 2023 • edited

ajwerner commented Jun 19, 2023 •

edited

netlify bot commented Jun 19, 2023 •

edited

brevilo commented Oct 12, 2023 •

edited