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

Tcx #921

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Tcx #921

wants to merge 1 commit into from

Conversation

astoycos
Copy link
Member

@astoycos astoycos commented Apr 5, 2024

Fixes #918

This initial attempt works with the following example -> https://github.com/astoycos/tcxtest

Please see integration tests for example of how the new API works


This change is Reviewable

@astoycos
Copy link
Member Author

astoycos commented Apr 5, 2024

cc @dave-tucker For some eyes

Copy link

netlify bot commented Apr 5, 2024

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d8f0595
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/66f8639efcdfc90008e33537
😎 Deploy Preview https://deploy-preview-921--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 configuration.

@mergify mergify bot added aya This is about aya (userspace) aya-bpf This is about aya-bpf (kernel) aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI labels Apr 5, 2024
Copy link

mergify bot commented Apr 5, 2024

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

@mergify mergify bot requested a review from alessandrod April 5, 2024 17:13
@mergify mergify bot added the api/needs-review Makes an API change that needs review label Apr 5, 2024
@alessandrod
Copy link
Collaborator

Fixes #918

Thanks so much for doing all this work!

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....

* For this first pass I exposed a tightly controlled way of ordering links that can be used anywhere since this API is expected to be expanded to other program types in the future,  i.e
/// Defines how to assign a link priorty when using the kernels mprog feature.
pub enum LinkOrdering {
    /// Assign the link as the first link.
    First,
    /// Assign the link as the last link.
    Last,
    /// Assign the link before the given link specified by it's file descriptor.
    BeforeLink(FdLink),
    /// Assign the link after the given link specified by it's file descriptor.
    AfterLink(FdLink),
    /// Assign the link before the given link specified by it's id.
    BeforeLinkId(u32),
    /// Assign the link after the given link specified by it's id.
    AfterLinkId(u32),
    /// Assign the link before the given program specified by it's file descriptor.
    BeforeProgram(Program),
    /// Assign the link after the given program specified by it's file descriptor.
    AfterProgram(Program),
    /// Assign the link before the given program specified by it's id.
    BeforeProgramID(u32),
    /// Assign the link after the given program specified by it's id.
    AfterProgramID(u32),
}

This seems fine, except the current definition isn't type safe:

LinkOrder::AfterProgram(KprobeProgram)

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 LinkOrdering::BeforeProgramId(42) is not possible, but LinkOrdering::BeforeProgramId(unsafe { ProgramId::new(42) }) is.

* The tc link is modeled very similar to our XdpLink i.e
#[derive(Debug)]
pub(crate) enum TcLinkInner {
    TcxLink(FdLink),
    NlLink(NlLink),
}

This also seems fine

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 🤔

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!

* To express the desire to load a tc program via TCX rather than via netlink currently the user must use `SchedClassifier.attach_with_options()` with the new options type
/// Options for SchedClassifer attach via TCX.
#[derive(Debug)]
pub struct TcxOptions {
    /// Ordering defines the tcx link ordering relative to other links on the
    /// same interface.
    pub ordering: LinkOrdering,
    /// Expected revision is used to ensure that tcx link order setting is not
    /// racing with another process.
    pub expected_revision: Option<u64>,
}

However I'm starting to think that deprecating attach_with_options, and removing the TcxOptions, and NlOptions structs would be better. Instead just having attach_tc(ARGS) and attach_netlink(ARGS) would be alot cleaner for the user, the standard attach can maintain the same behavior (default netlink) for now

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.

@astoycos
Copy link
Member Author

astoycos commented Jul 9, 2024

Hiya @alessandrod i'm FINALLY getting back to this :)

Everything looks good on your comments above except I do have some questions on...

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.

So the main problem with making this truly "transparent" is that the tcx requires new return codes https://github.com/torvalds/linux/blob/master/include/uapi/linux/bpf.h#L6413

/* (Simplified) user return codes for tcx prog type.
 * A valid tcx program must return one of these defined values. All other
 * return codes are reserved for future use. Must remain compatible with
 * their TC_ACT_* counter-parts. For compatibility in behavior, unknown
 * return codes are mapped to TCX_NEXT.
 */
enum tcx_action_base {
	TCX_NEXT	= -1,
	TCX_PASS	= 0,
	TCX_DROP	= 2,
	TCX_REDIRECT	= 7,
};

vs

#define TC_ACT_UNSPEC	(-1)
#define TC_ACT_OK		0
#define TC_ACT_RECLASSIFY	1
#define TC_ACT_SHOT		2
#define TC_ACT_PIPE		3
#define TC_ACT_STOLEN		4
#define TC_ACT_QUEUED		5
#define TC_ACT_REPEAT		6
#define TC_ACT_REDIRECT		7
#define TC_ACT_TRAP		8 /* For hw path, this means "trap to cpu"
				   * and don't further process the frame
				   * in hardware. For sw path, this is
				   * equivalent of TC_ACT_STOLEN - drop
				   * the skb and act like everything
				   * is alright.
				   */
#define TC_ACT_VALUE_MAX	TC_ACT_TRAP

So therefore technically the bytecode AND the attach mechanism has to change for TCX to work properly... However this does seem to be backwards compatible, i.e the old return codes will still work as expected so we should be implement

"Use TCX if available, if not fall back to netlink"

Let me take a stab

@astoycos
Copy link
Member Author

@alessandrod Can you PTAL another look here? I changed up the API a bit making it much more type safe :)

LMK what you think and if you're alright with it I'll continue by adding some integration tests before marking as ready for review

aya/src/programs/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 19 of 25 files reviewed, 13 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)


aya/src/programs/mod.rs line 697 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I was thinking an enum (none, FD, if index)

Done


aya/src/programs/mod.rs line 705 at r18 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

ack. i'll make query (or query_by_ifindex pending resolution of the other comment) take a u32 to avoid the conversion here. conversion can be handled by the caller in the same way as it is for SchedClassifier::attach.

Done


aya/src/programs/tc.rs line 360 at r18 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

no as this wouldn't be consistent with other Aya APIs that take an interface as a parameter. For example, aya::programs::SchedClassifier::attach

Done


aya/src/programs/tc.rs line 363 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

let (foo, bar) = ... to avoid .1 and .0 below

Done


aya/src/programs/tc.rs line 379 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: collect::<Result<_, _>>()? to avoid doing the error check below

Done


test/integration-test/src/tests/tcx.rs line 27 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

no it doesn't

Done


test/integration-test/src/tests/tcx.rs line 31 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Surely we can extract EbpfLoader::new()? this loader is also not a loader at all, it's a Ebpf, so this name is misleading.

Done


test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why not yield out program as well? that would remove the need for

let default_prog: &SchedClassifier = default.program("tcx_next").unwrap().try_into().unwrap();

below

This isn't possible since program is a borrowed from loader.


test/integration-test/src/tests/tcx.rs line 118 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

prefer to do the assertion on the outside so failures produce more information - as written this will stop after the first mismatch.

Done

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r19, 1 of 1 files at r20, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @alessandrod and @astoycos)


aya/src/sys/bpf.rs line 501 at r20 (raw file):

}

#[derive(Debug, Clone)]

does it need clone?


test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

This isn't possible since program is a borrowed from loader.

I think you mean it is borrowed from ebpf. Fine. Then can we yield out only program? That seems to be the only thing we actually need.


test/integration-test/src/tests/tcx.rs line 118 at r18 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

Done

why the string conversion? can't you write

    assert_eq!(
        got_order.iter().map(|p| p.id()).collect::<Vec<_>>(),
        expected_order
    );

?


test/integration-test/src/tests/tcx.rs line 111 at r20 (raw file):

    let (revision, got_order) = SchedClassifier::query_tcx("lo", TcAttachType::Ingress).unwrap();
    assert_eq!(got_order.len(), expected_order.len());

this is a pointless assertion now


test/integration-test/src/tests/tcx.rs line 112 at r20 (raw file):

    let (revision, got_order) = SchedClassifier::query_tcx("lo", TcAttachType::Ingress).unwrap();
    assert_eq!(got_order.len(), expected_order.len());
    assert_eq!(revision, 10);

magic number

Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)


aya/src/sys/bpf.rs line 501 at r20 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

does it need clone?

Done


test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think you mean it is borrowed from ebpf. Fine. Then can we yield out only program? That seems to be the only thing we actually need.

We still need to yield the link_id as it's used to test the LinkOrder::before_link and LinkOrder::after_link variants.
Granted, we only need one link_id in particular, but I couldn't reason an easier way to do this.


test/integration-test/src/tests/tcx.rs line 118 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why the string conversion? can't you write

    assert_eq!(
        got_order.iter().map(|p| p.id()).collect::<Vec<_>>(),
        expected_order
    );

?

Done

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r21, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alessandrod and @astoycos)


test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

We still need to yield the link_id as it's used to test the LinkOrder::before_link and LinkOrder::after_link variants.
Granted, we only need one link_id in particular, but I couldn't reason an easier way to do this.

Right, but link_id is a scalar, no? Perhaps you took me too literally, I meant "yield program instead of ebpf".

Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)


test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Right, but link_id is a scalar, no? Perhaps you took me too literally, I meant "yield program instead of ebpf".

Yielding only program won't work since it's borrowed from ebpf which gets dropped when we exit the closure (inside the macro). In borrow checker speak - 'ebpf' dropped here while still borrowed.
We could do that in future if someone where to implement #809

And no link_id is not scalar. It's a composite type.
Specifically it's a TcLinkIdInner::FdLinkId which is a tuple struct containing the RawFd returned from the bpf_prog_attach syscall. There's no easy way to get a list of links from either an ebpf or program. This is the best I can do with the APIs available.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alessandrod and @astoycos)


test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

Yielding only program won't work since it's borrowed from ebpf which gets dropped when we exit the closure (inside the macro). In borrow checker speak - 'ebpf' dropped here while still borrowed.
We could do that in future if someone where to implement #809

And no link_id is not scalar. It's a composite type.
Specifically it's a TcLinkIdInner::FdLinkId which is a tuple struct containing the RawFd returned from the bpf_prog_attach syscall. There's no easy way to get a list of links from either an ebpf or program. This is the best I can do with the APIs available.

The way you've written this confers no benefit to it being a macro rather than a function. Exploiting the fact that it is a macro we can write (something like):

diff --git a/test/integration-test/src/tests/tcx.rs b/test/integration-test/src/tests/tcx.rs
index f334a02..73dfea8 100644
--- a/test/integration-test/src/tests/tcx.rs
+++ b/test/integration-test/src/tests/tcx.rs
@@ -7,8 +7,8 @@ use test_log::test;
 
 use crate::utils::NetNsGuard;
 
-#[test(tokio::test)]
-async fn tcx() {
+#[test]
+fn tcx() {
     let kernel_version = KernelVersion::current().unwrap();
     if kernel_version < KernelVersion::new(6, 6, 0) {
         eprintln!("skipping tcx_attach test on kernel {kernel_version:?}");
@@ -26,49 +26,60 @@ async fn tcx() {
     //
     // Yields a tuple of the `Ebpf` which must remain in scope for the duration
     // of the test, and the link ID of the attached program.
-    macro_rules! attach_program_with_linkorder {
-        ($link_order:expr) => {{
+    macro_rules! attach_program_with_link_order {
+        ($program_name:ident, $link_id_name:ident, $link_order:expr) => {
             let mut ebpf = Ebpf::load(crate::TCX).unwrap();
-            let program: &mut SchedClassifier =
+            let $program_name: &mut SchedClassifier =
                 ebpf.program_mut("tcx_next").unwrap().try_into().unwrap();
-            program.load().unwrap();
-            let link_id = program
+            $program_name.load().unwrap();
+            let $link_id_name = $program_name
                 .attach_with_options(
                     "lo",
                     TcAttachType::Ingress,
                     TcAttachOptions::TcxOrder($link_order),
                 )
                 .unwrap();
-            (ebpf, link_id)
-        }};
+        };
     }
 
-    let (default, _) = attach_program_with_linkorder!(LinkOrder::default());
-    let (first, _) = attach_program_with_linkorder!(LinkOrder::first());
-    let (mut last, last_link_id) = attach_program_with_linkorder!(LinkOrder::last());
+    attach_program_with_link_order!(default, default_link_id, LinkOrder::default());
+    attach_program_with_link_order!(first, first_link_id, LinkOrder::first());
+    attach_program_with_link_order!(last, last_link_id, LinkOrder::last());
 
-    let default_prog: &SchedClassifier = default.program("tcx_next").unwrap().try_into().unwrap();
-    let first_prog: &SchedClassifier = first.program("tcx_next").unwrap().try_into().unwrap();
-    let last_prog: &mut SchedClassifier = last.program_mut("tcx_next").unwrap().try_into().unwrap();
+    let last_link = last.take_link(last_link_id).unwrap();
 
-    let last_link = last_prog.take_link(last_link_id).unwrap();
-
-    let (before_last, _) =
-        attach_program_with_linkorder!(LinkOrder::before_link(&last_link).unwrap());
-    let (after_last, _) =
-        attach_program_with_linkorder!(LinkOrder::after_link(&last_link).unwrap());
+    attach_program_with_link_order!(
+        before_last,
+        before_last_link_id,
+        LinkOrder::before_link(&last_link).unwrap()
+    );
+    attach_program_with_link_order!(
+        after_last,
+        after_last_link_id,
+        LinkOrder::after_link(&last_link).unwrap()
+    );
 
-    let (before_default, _) =
-        attach_program_with_linkorder!(LinkOrder::before_program(default_prog).unwrap());
-    let (after_default, _) =
-        attach_program_with_linkorder!(LinkOrder::after_program(default_prog).unwrap());
+    attach_program_with_link_order!(
+        before_default,
+        before_default_link_id,
+        LinkOrder::before_program(default_prog).unwrap()
+    );
+    attach_program_with_link_order!(
+        after_default,
+        after_default_link_id,
+        LinkOrder::after_program(default_prog).unwrap()
+    );
 
-    let (before_first, _) = attach_program_with_linkorder!(LinkOrder::before_program_id(unsafe {
-        ProgramId::new(first_prog.info().unwrap().id())
-    }));
-    let (after_first, _) = attach_program_with_linkorder!(LinkOrder::after_program_id(unsafe {
-        ProgramId::new(first_prog.info().unwrap().id())
-    }));
+    attach_program_with_link_order!(
+        before_first,
+        before_first_link_id,
+        LinkOrder::before_program_id(unsafe { ProgramId::new(first.info().unwrap().id()) })
+    );
+    attach_program_with_link_order!(
+        after_first,
+        after_first_link_id,
+        LinkOrder::after_program_id(unsafe { ProgramId::new(first.info().unwrap().id()) })
+    );
 
     let expected_order = [
         before_first

I wasn't able to get the integration tests compiling locally sadly, so this probably doesn't quite compile, but it should be close.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r22, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alessandrod and @astoycos)


test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

The way you've written this confers no benefit to it being a macro rather than a function. Exploiting the fact that it is a macro we can write (something like):

diff --git a/test/integration-test/src/tests/tcx.rs b/test/integration-test/src/tests/tcx.rs
index f334a02..73dfea8 100644
--- a/test/integration-test/src/tests/tcx.rs
+++ b/test/integration-test/src/tests/tcx.rs
@@ -7,8 +7,8 @@ use test_log::test;
 
 use crate::utils::NetNsGuard;
 
-#[test(tokio::test)]
-async fn tcx() {
+#[test]
+fn tcx() {
     let kernel_version = KernelVersion::current().unwrap();
     if kernel_version < KernelVersion::new(6, 6, 0) {
         eprintln!("skipping tcx_attach test on kernel {kernel_version:?}");
@@ -26,49 +26,60 @@ async fn tcx() {
     //
     // Yields a tuple of the `Ebpf` which must remain in scope for the duration
     // of the test, and the link ID of the attached program.
-    macro_rules! attach_program_with_linkorder {
-        ($link_order:expr) => {{
+    macro_rules! attach_program_with_link_order {
+        ($program_name:ident, $link_id_name:ident, $link_order:expr) => {
             let mut ebpf = Ebpf::load(crate::TCX).unwrap();
-            let program: &mut SchedClassifier =
+            let $program_name: &mut SchedClassifier =
                 ebpf.program_mut("tcx_next").unwrap().try_into().unwrap();
-            program.load().unwrap();
-            let link_id = program
+            $program_name.load().unwrap();
+            let $link_id_name = $program_name
                 .attach_with_options(
                     "lo",
                     TcAttachType::Ingress,
                     TcAttachOptions::TcxOrder($link_order),
                 )
                 .unwrap();
-            (ebpf, link_id)
-        }};
+        };
     }
 
-    let (default, _) = attach_program_with_linkorder!(LinkOrder::default());
-    let (first, _) = attach_program_with_linkorder!(LinkOrder::first());
-    let (mut last, last_link_id) = attach_program_with_linkorder!(LinkOrder::last());
+    attach_program_with_link_order!(default, default_link_id, LinkOrder::default());
+    attach_program_with_link_order!(first, first_link_id, LinkOrder::first());
+    attach_program_with_link_order!(last, last_link_id, LinkOrder::last());
 
-    let default_prog: &SchedClassifier = default.program("tcx_next").unwrap().try_into().unwrap();
-    let first_prog: &SchedClassifier = first.program("tcx_next").unwrap().try_into().unwrap();
-    let last_prog: &mut SchedClassifier = last.program_mut("tcx_next").unwrap().try_into().unwrap();
+    let last_link = last.take_link(last_link_id).unwrap();
 
-    let last_link = last_prog.take_link(last_link_id).unwrap();
-
-    let (before_last, _) =
-        attach_program_with_linkorder!(LinkOrder::before_link(&last_link).unwrap());
-    let (after_last, _) =
-        attach_program_with_linkorder!(LinkOrder::after_link(&last_link).unwrap());
+    attach_program_with_link_order!(
+        before_last,
+        before_last_link_id,
+        LinkOrder::before_link(&last_link).unwrap()
+    );
+    attach_program_with_link_order!(
+        after_last,
+        after_last_link_id,
+        LinkOrder::after_link(&last_link).unwrap()
+    );
 
-    let (before_default, _) =
-        attach_program_with_linkorder!(LinkOrder::before_program(default_prog).unwrap());
-    let (after_default, _) =
-        attach_program_with_linkorder!(LinkOrder::after_program(default_prog).unwrap());
+    attach_program_with_link_order!(
+        before_default,
+        before_default_link_id,
+        LinkOrder::before_program(default_prog).unwrap()
+    );
+    attach_program_with_link_order!(
+        after_default,
+        after_default_link_id,
+        LinkOrder::after_program(default_prog).unwrap()
+    );
 
-    let (before_first, _) = attach_program_with_linkorder!(LinkOrder::before_program_id(unsafe {
-        ProgramId::new(first_prog.info().unwrap().id())
-    }));
-    let (after_first, _) = attach_program_with_linkorder!(LinkOrder::after_program_id(unsafe {
-        ProgramId::new(first_prog.info().unwrap().id())
-    }));
+    attach_program_with_link_order!(
+        before_first,
+        before_first_link_id,
+        LinkOrder::before_program_id(unsafe { ProgramId::new(first.info().unwrap().id()) })
+    );
+    attach_program_with_link_order!(
+        after_first,
+        after_first_link_id,
+        LinkOrder::after_program_id(unsafe { ProgramId::new(first.info().unwrap().id()) })
+    );
 
     let expected_order = [
         before_first

I wasn't able to get the integration tests compiling locally sadly, so this probably doesn't quite compile, but it should be close.

I have made this change.

@tamird
Copy link
Member

tamird commented Sep 22, 2024

Reminder to please squash on or before merge.

@dave-tucker
Copy link
Member

Thanks for the reminder @tamird. Commits squashed.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r23, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alessandrod and @astoycos)

aya/src/programs/links.rs Outdated Show resolved Hide resolved
/// # Example
///
///```no_run
/// # let mut bpf = aya::Ebpf::load(&[])?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// # let mut bpf = aya::Ebpf::load(&[])?;
/// # let mut ebpf = aya::Ebpf::load(&[])?;

Copy link
Member

Choose a reason for hiding this comment

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

We're still let mut bpf in all other examples. I'd prefer to stick to the established pattern.. or change all of them to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Fair, let's change them in a separate PR then.

anfredette added a commit to anfredette/bpfman that referenced this pull request Sep 26, 2024
aya-rs/aya#921

Do not merge in bpfman untl the tcx support gets merged in Aya.

Signed-off-by: Andre Fredette <[email protected]>

/// A checked integral type conversion failed.
#[error(transparent)]
TryFromIntError(#[from] TryFromIntError),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't do this. We're exposing an implementation detail to the public
API: what if we stop using try_from/try_into in the implementation? Or if I get
the error, I get "TryFromIntError" with an error "an integral type conversion
failed" - what? what did I do wrong? how do I fix it?

Where is this used from? If we must fail, we should use a better, higher level
error variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve made the error more descriptive.

I’m only using it in cases where an if index is being converted from u32 to i32. The problem stems from the fact that the kernel isn’t very disciplined about whether to use a u32 or i32 for if index, so we need to convert sometimes. The existing code was using the unchecked as i32 to do the conversion, but Tamir wanted that changed to a checked try_into.

What happens if it fails? In these cases, it should never fail because the value should have started out as an i32 in the kernel, but got converted to a u32 somewhere before we got it, so it should convert back to an i32 just fine. In the unlikely event that it does fail, there’s a bug either in our code or in one of the external Linux functions, and the user would need to report it and/or fix it.

@@ -239,6 +245,20 @@ impl AsFd for ProgramFd {
}

/// The various eBPF programs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this line was left here accidentally

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed.

@@ -797,6 +819,57 @@ impl_fd!(
CgroupDevice,
);

/// Defines the [`Program`] types which support the kernel's
Copy link
Collaborator

Choose a reason for hiding this comment

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

A trait does not define.

Trait implemented by the [`Program`] types which support ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.


impl_multiprog_fd!(SchedClassifier);

/// Defines the [`Link`] types which support the kernel's
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

/// # Minimum kernel version
///
/// The minimum kernel version required to use this feature is 6.6.0.
pub trait MultiProgProgram {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MultiProgProgram kinda stutters... maybe MultiProgram?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, and MultiProgram sounds better. Done.

match self {
Self::Ingress => Ok(BPF_TCX_INGRESS),
Self::Egress => Ok(BPF_TCX_EGRESS),
Self::Custom(tcx_attach_type) => Err(TcError::InvalidTcxAttach(*tcx_attach_type)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would return None in this case. Also TcAttachType::parent should return None
for tcx I think? Tcx programs use LinkOrder I don't think that parent makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

See below.

InvalidTcxAttach(u32),
/// operation not supported for programs loaded via tcx
#[error("operation not supported for programs loaded via tcx")]
InvalidLinkOperation,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would remove obth this and InvalidTcxAttach and return None where necessary
instead. The way things are now, TcAttachType::parent returns "something" but
it's not really a valid value for TCX. Instead of adding another error for that
case, I'd just return None everywhere.

Copy link
Contributor

@anfredette anfredette Sep 27, 2024

Choose a reason for hiding this comment

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

As I’ve worked with the code in this pr and in my bpfman implementation I’ve started thinking that it might be better to have different attach types for TC and TCX because they are really two different hook points and attach types with different kernel APIs and attributes. Trying to treat them as the same adds complication as you are pointing out here. Since I wasn’t involved in the earlier design discussions, what’s implemented works, and it’s kind of late in the game, I was planning to see if I could come up with a cleaner solutions after this pr merges. That said, with the current design, I think it’s best to leave these errors as-is for now. Some details follow.

TcAttachType::tcx_attach_type() should only be called from a TCX context, and the attach type should never be Custom. If the user makes an error, it's better to return an error here rather than requiring the caller to handle a None error case later.

TcAttachType::parent should only be used in a TC (netlink) context, which it currently is. However, we can't detect if it's mistakenly used in a TCX context so we can’t return something different for TCX. To help clarify, I renamed parent to tc_parent to indicate that it applies only to TC.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r24, 4 of 4 files at r25, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @alessandrod, @anfredette, @astoycos, @dave-tucker, and @vadorovsky)


aya/src/programs/mod.rs line 222 at r25 (raw file):

    /// Value is too large to use as an interface index.
    #[error("Value {value} is too large to use as an interface index")]

nit: the other error strings aren't capitalized


aya/src/programs/mod.rs line 225 at r25 (raw file):

    InvalidIfIndex {
        /// Value used in conversion attempt.
        value: u32,

nit: s/value/if_index/?


aya/src/programs/mod.rs line 823 at r25 (raw file):

);

/// Trait implemented by the [`Program`] types which support the kernel's

nit: don't say "trait"? the code already says it (ditto below)

@alessandrod
Copy link
Collaborator

Reviewed 1 of 1 files at r24, 4 of 4 files at r25, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @alessandrod, @anfredette, @astoycos, @dave-tucker, and @vadorovsky)

aya/src/programs/mod.rs line 222 at r25 (raw file):

    /// Value is too large to use as an interface index.
    #[error("Value {value} is too large to use as an interface index")]

nit: the other error strings aren't capitalized

They should all be capitalized

aya/src/programs/mod.rs line 225 at r25 (raw file):

    InvalidIfIndex {
        /// Value used in conversion attempt.
        value: u32,

nit: s/value/if_index/?

aya/src/programs/mod.rs line 823 at r25 (raw file):

);

/// Trait implemented by the [`Program`] types which support the kernel's

nit: don't say "trait"? the code already says it (ditto below)

I would rather say it. In general, I would always do what std does eg https://doc.rust-lang.org/stable/std/borrow/trait.Borrow.html

@@ -150,11 +205,11 @@ impl SchedClassifier {
&mut self,
interface: &str,
attach_type: TcAttachType,
options: TcOptions,
options: TcAttachOptions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this api it's now possible to

attach_with_options("foo", TcAttachType::Custom(42), TcAttachOptions::TcxOrder(...))

which shouldn't be possible.

How badly blocked is bpfman on this? This PR has been going on for a while, so I
understand if you want to merge asap to unblock bpfman.

If it's not super urgent, I think this whole TcAtachType/TcAttachOptions needs
to be redone, I can try to come up with a better API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, let's just merge as is, with the errors instead of None. No need to
block this further I'll clean up the API later.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alessandrod It's perfectly fine with me to merge it as is. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

And, I'd be happy to work on an improved API later.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the risk of extending this further, I pushed a couple more changes that we had discussed:

  1. Changed the two try_into's for i32 to "as i32" and deleted the InvalidIfIndex error. (I already had the change staged, and I think it's better.)
  2. Don't try to attach as TCX for TcAttachType::Custom. (This was a bug, so I thought it was worth fixing.)

This commit adds the initial support for TCX
bpf links. This is a new, multi-program, attachment
type allows for the caller to specify where
they would like to be attached relative to other
programs at the attachment point using the LinkOrder
type.

Signed-off-by: astoycos <[email protected]>
Co-authored-by: Andre Fredette <[email protected]>
Co-authored-by: Dave Tucker <[email protected]>
Co-authored-by: Tamir Duberstein <[email protected]>
Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

LGTM

@dave-tucker
Copy link
Member

@alessandrod Is this ready to go now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/needs-review Makes an API change that needs review aya This is about aya (userspace) aya-bpf This is about aya-bpf (kernel) aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TCX link support
7 participants