Skip to content
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

feat(s2n-quic-xdp): add ring module #1706

Merged
merged 3 commits into from
Apr 15, 2023
Merged

feat(s2n-quic-xdp): add ring module #1706

merged 3 commits into from
Apr 15, 2023

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Apr 13, 2023

Description of changes:

This change adds the XDP ring data structure for coordinating with the kernel. Most of this code is based on the xsk.h file from the xdp-project.

Each ring type (TX, RX, Fill, Comp) wraps the generic Ring structure and exports a well-typed API.

Call-outs:

I considered generating bindings from the libxdp project for this. However, the implementation is all contained in the header, which bindgen has a hard time with - none of the meaningful functions are actually exposed. This means this logic has to be either wrapped in a C module or it needs to be ported to Rust. In this case I opted for porting it, since we'd potentially incurring runtime costs without cross-language LTO enabled. It also keeps the build simpler.

Testing:

I added a bolero test for the ring cursor implementation. It defines a FIFO queue as an oracle and ensures the behavior of the ring matches.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft marked this pull request as ready for review April 13, 2023 03:40
///
/// See [xsk.h](https://github.com/xdp-project/xdp-tools/blob/a76e7a2b156b8cfe38992206abe9df1df0a29e38/headers/xdp/xsk.h#L34-L42).
#[derive(Debug)]
pub struct Cursor<T: Copy + fmt::Debug> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you match the ordering in the xsk. Also desc -> ring. Finally a brief comment on cached_len and why you added it for optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to preserve the desc rather than ring. Ring is just used all over the place and I wanted to say that this points to the descriptors.

I can add the docs and reorder, though.

Comment on lines 154 to 168
/// Returns the cached producer cursor which is also maxed by the cursor mask
///
/// See [xsk.h](https://github.com/xdp-project/xdp-tools/blob/a76e7a2b156b8cfe38992206abe9df1df0a29e38/headers/xdp/xsk.h#L60).
#[inline]
pub fn cached_producer(&self) -> u32 {
self.cached_producer.0 & self.mask
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you update this documentation to mention the wrapping and modulo operation and power of 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

Comment on lines +331 to +345
ConsumerAcquire(u16),
ConsumerRelease(u16),
ProducerAcquire(u16),
ProducerRelease(u16),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would testing with u32 be useful since some of the values on Cursor are u32

Copy link
Contributor Author

@camshaft camshaft Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this case since we're limiting the queue size for the sake of memory usage. Having the smaller lengths also consumes less of the fuzzer input so the corpus is smaller as well.

.with_generator((1..=10, gen::<Vec<Op>>()))

But functionally, as long as we're testing a few queue sizes the behavior should all be the same.

@camshaft camshaft merged commit b558adf into main Apr 15, 2023
@camshaft camshaft deleted the camshaft/xsk-ring branch April 15, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants