Fix cgroup-skb-egress for new perf event array API#268
Conversation
✅ Deploy Preview for aya-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on Brskt).
examples/cgroup-skb-egress/cgroup-skb-egress/src/main.rs line 73 at r1 (raw file):
PerfEvent::Sample { head, tail } => { // Samples can straddle the ring's wrap boundary; copy a contiguous window. const N: usize = size_of::<PacketLog>();
PacketLog is Pod, right? Currently we are copying this twice: once from the sample to the stack buffer and then again into the PacketLog. Perhaps instead we could use MaybeUnint<PacketLog>? We would probably want https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.as_bytes_mut here but that's unstable so we can leave a TODO and use a handwritten implementation for now.
WDYT?
327e1fc to
39094f8
Compare
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on tamird).
examples/cgroup-skb-egress/cgroup-skb-egress/src/main.rs line 73 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
PacketLog is
Pod, right? Currently we are copying this twice: once from the sample to the stack buffer and then again into the PacketLog. Perhaps instead we could useMaybeUnint<PacketLog>? We would probably want https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.as_bytes_mut here but that's unstable so we can leave a TODO and use a handwritten implementation for now.WDYT?
Sure, looks good via Pod. Done with a small as_bytes_mut shim and a TODO.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on Brskt).
examples/cgroup-skb-egress/cgroup-skb-egress/src/main.rs line 103 at r2 (raw file):
); data.assume_init() };
Could we do something like
let bytes = as_bytes_mut(&mut data);
for (src, dst) in head.iter().chain(tail.iter()).zip(bytes) {
dst.write(src);
}
otherwise if you want to copy through the pointer then you can just write
let mut data = MaybeUninit::<PacketLog>::uninit();
let dst = data.as_mut_ptr().cast::<u8>();
...
Code quote:
let bytes = as_bytes_mut(&mut data);
let head_len = head.len().min(bytes.len());
let dst = bytes.as_mut_ptr().cast::<u8>();
// SAFETY: source and destination are non-overlapping, and the writes
// cover all of `bytes`; PacketLog is Pod so any byte pattern is valid.
let data = unsafe {
std::ptr::copy_nonoverlapping(
head.as_ptr(),
dst,
head_len,
);
std::ptr::copy_nonoverlapping(
tail.as_ptr(),
dst.add(head_len),
bytes.len() - head_len,
);
data.assume_init()
};aya replaced PerfEventArrayBuffer::read_events with closure-based
try_fold/fold/for_each that yields PerfEvent::Sample{head, tail}
borrowed from the kernel ring. Drain the buffer in one closure call
and write the wrap-aware payload into a MaybeUninit<PacketLog> before
calling assume_init, since samples can straddle the ring's wrap boundary.
Drop the bytes dependency (BytesMut was the only consumer) and surface
PerfEvent::Lost via warn!.
39094f8 to
3d10076
Compare
Brskt
left a comment
There was a problem hiding this comment.
@Brskt made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on tamird).
examples/cgroup-skb-egress/cgroup-skb-egress/src/main.rs line 103 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Could we do something like
let bytes = as_bytes_mut(&mut data); for (src, dst) in head.iter().chain(tail.iter()).zip(bytes) { dst.write(src); }otherwise if you want to copy through the pointer then you can just write
let mut data = MaybeUninit::<PacketLog>::uninit(); let dst = data.as_mut_ptr().cast::<u8>(); ...
Yeah, the iterator path is cleaner. Done with a debug_assert_eq! for the kernel invariant.
tamird
left a comment
There was a problem hiding this comment.
Thanks!
@tamird reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Brskt).
PR aya-rs/aya#1554 replaced
read_eventswithfor_each; thecgroup-skb-egress example no longer compiles. Migrate to the new
closure API and concat
head + tailto handle samples that wrapthe ring boundary.
This change is