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 if_xdp module #1702

Merged
merged 2 commits into from
Apr 11, 2023
Merged

feat(s2n-quic-xdp): add if_xdp module #1702

merged 2 commits into from
Apr 11, 2023

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Apr 10, 2023

Description of changes:

This PR:

  • initializes the s2n-quic-xdp crate. This will be the crate applications pull in to enable AF-XDP support in the IO provider.
  • Adds the if_xdp.rs module, which implements the structs, flags, and definitions in the kernel.

Call-outs:

The if_xdp could probably have been auto-generated with bindgen. However, the macro definitions aren't correctly translated, since they don't have an explicit type. I also wanted to make it a bit more ergonomic by using the bitflags crate, expanding the struct names/fields, and adding some additional documentation and references.

I also regenerated the bpf code while I was here, as a few dependencies have been updated since they were last committed.

Testing:

The xdp CI task now includes a cargo test and cargo clippy, which will build the s2n-quic-xdp crate and run any associated tests.

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

@camshaft camshaft force-pushed the camshaft/if_xdp branch 2 times, most recently from 8510776 to 3636b73 Compare April 10, 2023 22:03
@camshaft camshaft marked this pull request as ready for review April 10, 2023 22:20
.github/workflows/ci.yml Show resolved Hide resolved
Comment on lines +37 to +41
#[derive(Copy, Clone, Debug, Default)]
#[repr(transparent)]
pub struct UmemFlags: u32 {
const UNALIGNED_CHUNK_FLAG = 1 << 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this sized as u32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the flags are u32. You can also see the config field in libxdp. I can link this if that's helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand correctly, this is specifically matching the xdp-project interface. Yea it would be good to include a link to the xdp-project github, since this is definitely new territory for me and possible others.

#[repr(C)]
pub struct Address {
pub family: u16,
pub flags: XdpFlags,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

tools/xdp/s2n-quic-xdp/src/if_xdp.rs Show resolved Hide resolved
tools/xdp/s2n-quic-xdp/src/if_xdp.rs Show resolved Hide resolved
/// Dropped due to invalid descriptor
pub rx_invalid_descriptors: u64,
/// Dropped due to invalid descriptor
pub tx_invalid_descriptors: u32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub tx_invalid_descriptors: u32,
pub tx_invalid_descriptors: u64,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch. that would have been annoying to debug 😄

tools/xdp/s2n-quic-xdp/src/if_xdp.rs Show resolved Hide resolved
@camshaft camshaft requested a review from toidiu April 11, 2023 17:29
@camshaft camshaft merged commit bf20c6d into main Apr 11, 2023
@camshaft camshaft deleted the camshaft/if_xdp branch April 11, 2023 18:07
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