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

XDP Multi-Buffer Support #519

Merged
merged 2 commits into from
Feb 9, 2023
Merged

XDP Multi-Buffer Support #519

merged 2 commits into from
Feb 9, 2023

Conversation

dave-tucker
Copy link
Member

Fixes: #516

This adds support for loading XDP programs that are multi-buffer
capable, which is signalled using the xdp.frags section name. When this
is set, we should set the BPF_F_XDP_HAS_FRAGS flag when loading the
program into the kernel.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@netlify
Copy link

netlify bot commented Feb 9, 2023

Deploy Preview for aya-rs-docs ready!

Name Link
🔨 Latest commit 376c486
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/63e559ac1d081b00082aebf5
😎 Deploy Preview https://deploy-preview-519--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@brevilo
Copy link

brevilo commented Feb 9, 2023

Awesome, that was fast! Looks good from my point of view 👍

One minor comment: I wonder if multibuffer is the ideal attribute name. Yes, technically it's the correct term but devs coming from the original C implementation (docs, examples, kernel) might expect frags due to the original program section (xdp.frags) and symbol names used. I don't have a strong opinion on this, though, and multibuffer could be easier to understand (if well-documented) than the abbreviated frags.

@brevilo brevilo mentioned this pull request Feb 9, 2023
2 tasks
@@ -14,7 +14,7 @@ static FOO: Array<u32> = Array::<u32>::with_max_entries(10, 0);
#[map(name = "BAR")]
static BAZ: Array<u32> = Array::<u32>::with_max_entries(10, 0);

#[xdp]
#[xdp(multibuffer = "true")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we shouldn't switch all tests to use frags. It's unlikely but just in case we end up breaking the non-frags case.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack. we still have 2 aya xdp programs in the integration tests that will ensure we support the non-frags case too... and any of the C progs used in integration tests are just plain old xdp

Copy link
Collaborator

@alessandrod alessandrod left a comment

Choose a reason for hiding this comment

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

Just a couple of nits, otherwise amazing thanks so much!

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@dave-tucker
Copy link
Member Author

@alessandrod fixed all comments except for the note re: tests since we've still got good coverage on integration tests for the frags and non-frags cases.

@alessandrod alessandrod merged commit bc83f20 into aya-rs:main Feb 9, 2023
@brevilo
Copy link

brevilo commented Feb 10, 2023

Thanks guys, you rock! 👏

@dave-tucker dave-tucker added feature A PR that implements a new feature or enhancement aya This is about aya (userspace) labels Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace) feature A PR that implements a new feature or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What's missing for (pure) xdp.frags support?
3 participants