-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Linux: update and add missing AF_XDP API #3956
Conversation
r? @JohnTitor rustbot has assigned @JohnTitor. Use |
Thanks for the PR, I'm taking a look at this. Where is
The kernel removed the old interface because it was buggy, right? If the kernel made the decision to do a breaking change here then I think we can just do the same. |
Thank you!
Correct, the issue was initially reported here with the fix removing the |
☔ The latest upstream changes (presumably #4018) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #4092) made this pull request unmergeable. Please resolve the merge conflicts. |
src/unix/linux_like/linux/gnu/mod.rs
Outdated
@@ -433,6 +434,15 @@ s! { | |||
pub options: ::__u32, | |||
} | |||
|
|||
pub struct xsk_tx_metadata_completion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything added here is uapi right? It should probably go in src/unix/linux_like/linux/mod.rs
rather than duplicating into gnu and musl. (The existing xdp types could probably move as well? Not in this PR though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything added here is uapi right?
Yes.
The existing xdp types could probably move as well?
I think so, I wasn't sure where to put them when I initially added them. I'll create a PR after this one is merged to move the rest too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a bit of looking and it seems like rustix
is about the only consumer of the specific type that would break. I asked about possible fallout here bytecodealliance/rustix#1221, if they say they can work around us breaking things then that is much easier than figuring out the best mitigation here.
In any case we can get this merged to main
, left a comment about deduplicating gnu and musl.
@rustbot author
ce365fa
to
6bc41c9
Compare
Everything that was added in this PR (besides the field addition) is moved to @rustbot review |
Thanks for moving things. I'll merge this now but hold off for a few more days on the backport to hear back from rustix (I am leaning toward just doing the backport, we can't reasonably do a major release in every case where the API we bind breaks). |
(backport <rust-lang#3956>) (cherry picked from commit 6bc41c9)
(backport <rust-lang#3956>) (cherry picked from commit 88aa42a)
Description
This PR adds missing structs/constants from the if_xdp.h header and updates the
xdp_umem_reg
struct. Due to the added field inxdp_umem_reg
, this is a breaking change from the 0.2 branch.Ideally, I would like to include all the changes in the 0.2 branch. Would it be possible to add a
xdp_umem_reg_v3
to not break the currentxdp_umem_reg
?If not, how would deprecating a struct to change it work? Just add a deprecation notice, wait for X amount of time, add the field?
Sources
AF_XDP documentation: https://docs.kernel.org/networking/af_xdp.html
Header file: https://github.com/torvalds/linux/blob/v6.11/include/uapi/linux/if_xdp.h
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI
I could not test the the changes locally due to the error described in #3865 (comment)