-
Notifications
You must be signed in to change notification settings - Fork 251
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
Tcx #921
base: main
Are you sure you want to change the base?
Tcx #921
Conversation
cc @dave-tucker For some eyes |
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
73ad791
to
43b67e4
Compare
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
Add the return codes defined in the bpf.h `tcx_action_base` enum. This allows users to write tc prograns which correctly interact with the new tcx api. Signed-off-by: astoycos <[email protected]>
This commit adds the initial support for tcx bpf links. Signed-off-by: astoycos <[email protected]>
Signed-off-by: astoycos <[email protected]>
bfe585e
to
fc9f947
Compare
Thanks so much for doing all this work!
This seems fine, except the current definition isn't type safe:
This shouldn't be possible. We should try to make the API as type safe as possible. Among other things we should probably have a wrapper type for program ids, so that
This also seems fine
I don't think we can do this without giving up on a lot of type safety. If you have only one Link type, then all operations become possilble on all links: xdp operations on tc links, kprobe operations on xdp links, etc. But if you have something in mind that would work I'd love to see it!
As an user of aya, I probably don't know anything about netlink or TCX and I shouldn't really learn anything about it. I think we should make this as transparent as possible to users. Like we do for xdp, we should probably detect whether we can attach as TCX or fallback to netlink, and in the default/common case, have sensible defaults for LinkOrdering and everything else. |
Fixes #918
This initial attempt works with the following example -> https://github.com/astoycos/tcxtest
I'm not at all happy with the API yet so leaving this as a draft for now, once we iron that out I'll also implement integration tests (which would be much easier with the work from #915)
Some open design notes/questions....
However the way we model links seems to be becoming a bit messy and I'm wondering if we could refactor this to have a simple generic
Link
type rather then having every program have specific variants 🤔SchedClassifier.attach_with_options()
with the new options typeHowever I'm starting to think that deprecating
attach_with_options
, and removing theTcxOptions
, andNlOptions
structs would be better. Instead just havingattach_tc(ARGS)
andattach_netlink(ARGS)
would be alot cleaner for the user, the standardattach
can maintain the same behavior (default netlink) for nowBasic testing just exercises First/Last