Support using handle in tc programs #418

anfredette · 2022-10-17T19:57:58Z

Implements step 1 of #414.

Adds handle to the SchedClassifier attach API
Saves handle in the TcLink sruct and uses it when detaching programs

NOTE: This pr changes the API, so I think it will require a bump in the Aya version.

Signed-off-by: Andre Fredette afredette@redhat.com

netlify · 2022-10-17T19:58:02Z

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`abb75ba`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/6376b8734528710008eec935
😎 Deploy Preview	https://deploy-preview-418--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

vadorovsky

The code LGTM, just 2 nitpicks about comments.

aya/src/programs/tc.rs

aya/src/sys/netlink.rs

anfredette · 2022-10-19T13:49:23Z

The last update addressed @vadorovsky's comments.

dave-tucker · 2022-10-20T14:06:25Z

/cc @alessandrod this touches the API so needs your LGTM

alessandrod · 2022-10-20T15:19:00Z

aya/src/programs/tc.rs

@@ -124,19 +127,28 @@ impl SchedClassifier {
        interface: &str,


I think we can probably make the API a bit nicer. Now most people will be calling it like this (as per our own doc example):

let prog: &mut SchedClassifier = bpf.program_mut("redirect_ingress").unwrap().try_into()?; prog.load()?; prog.attach("eth0", TcAttachType::Ingress, 0, 0)?;

That "0, 0" isn't great. Maybe we should have:

prog.attach("eth0", TcAttachType::Ingress);

and for people that want more control

prog.attach_with_options("eth0", TcAttachType::Ingress, priority, handle)

Suggestions for alternatives to the attach_with_options name welcome

I don't like the 0's either. We could make this more readable by using constants. For example:

prog.attach("eth0", TcAttachType::Ingress, tc::AUTO_PRI, tc::AUTO_HNDL)?;

I think whether we should have two versions depends on whether most users would want to explicitly use the options. If they don't then it makes sense to have the "simple" and "advanced" versions.

I suspect that many would want to set the priority, but most would not need to choose the handle. What do you think?

Having the constants like that wouldn't be very rusty I think. If we wanted to go that route, we could have

#[derive(Default)] struct TcAttachPriority(u32); #[derive(Default)] struct TcAttachHandle(u32);

And then you would call it with attach("eth0", TcAttachType::Ingress, TcAttachPriority::default(), TcAttachHandle::default()), or attach("eth0", TcAttachType::Ingress, TcAttachPriority(123), TcAttachHandle(345)). It still seems a bit much to me.

I have personally never passed priority and handle, so to me two methods seems the best approach. But I've never had to integrate with any other TC programs, I genuinely don't know how common that is.

If we went the TcAttachPriotity and TcAttachHandle way we could actually have

fn attach<P: Into<TcAttachPriotity>, H: Into<TcAttachHandle>>(name: &str, attach_type: TcAttachType, priority: P, handle: H) -> ... { let priority = priority.into(); let handle = handle.into(); ... }

and then call it as

attach("eth0", TcAttachType::Ingress, TcAttachPriority::default(), TcAttachHandle::default())

or

attach("eth0", TcAttachType::Ingress, 123, 456)

which is more characters but better than 0, 0

I think this is now my preference :)

That works for me. Thanks.

But, I'll hold off until tomorrow to see if anyone else has other ideas :-)

Implements step 1 of aya-rs#414. - Adds handle to the SchedClassifier attach API - Saves handle in the TcLink sruct and uses it when detaching programs NOTE: this changes the API, so it will require a bump in the Aya version. Signed-off-by: Andre Fredette <afredette@redhat.com>

anfredette · 2022-10-24T17:31:56Z

Update: The last force push was just a rebase on main.

Use a struct called TcOptions for setting priority and handle in SchedClassifier attach struct TcOptions implements the Default trait, so for the simple use case in which the defaults are acceptable, we can call attach as follows: attach(“eth0”, TcAttachType::Ingress, TcOptions::default()) To specify all options: attach(“eth0”, TcAttachType::Ingress, TcOptions { priority: (50), handle: (3) }) Or, some options: attach(“eth0”, TcAttachType::Ingress, TcOptions { priority: (50), ..Default::default() }) Signed-off-by: Andre Fredette <afredette@redhat.com>

anfredette · 2022-10-24T21:43:16Z

@alessandrod, I thought about this a bit and have pushed a commit with another option for the priority/handle API that uses a struct called TcOptions for setting the priority and handle in attach

TcOptions implements the Default trait, so for the simple use case in which the defaults are acceptable, we can call attach as follows:
attach(“eth0”, TcAttachType::Ingress, TcOptions::default())

To specify all options:
attach(“eth0”, TcAttachType::Ingress, TcOptions { priority: (50), handle: (3) })

Or, some options:
attach(“eth0”, TcAttachType::Ingress, TcOptions { priority: (50), ..Default::default() })

An advantage of this approach is that we can add more defaults in the future and the user code doesn’t need to change. For example, we currently assume that the tc programs will be attached to the clsact QDISC. However, someone may have a use case for attaching to other QDISC’s in the future and that could be added as an option.

Please let me know what you think.

dave-tucker · 2022-11-02T15:25:40Z

@alessandrod This LGTM. Can you take a look over the new API?

alessandrod · 2022-11-02T17:05:39Z

@alessandrod, I thought about this a bit and have pushed a commit with another option for the priority/handle API that uses a struct called TcOptions for setting the priority and handle in attach

Thank you for working on this while we figure out the best API! 😊

TcOptions implements the Default trait, so for the simple use case in which the defaults are acceptable, we can call attach as follows: attach(“eth0”, TcAttachType::Ingress, TcOptions::default())

To specify all options: attach(“eth0”, TcAttachType::Ingress, TcOptions { priority: (50), handle: (3) })

Or, some options: attach(“eth0”, TcAttachType::Ingress, TcOptions { priority: (50), ..Default::default() })

An advantage of this approach is that we can add more defaults in the future and the user code doesn’t need to change. For example, we currently assume that the tc programs will be attached to the clsact QDISC. However, someone may have a use case for attaching to other QDISC’s in the future and that could be added as an option.

I don't think bpf programs can be attached to anything other than clsact? There's the old style way of doing cls and act separately but that's pretty much deprecated now that one program can do both.

Please let me know what you think.

This API seems pretty un-rusty to me. If we had many options, I think it would make more sense. But as it is it looks a bit like a way to smuggle named arguments (keywords) where the language doesn't support them. Why is TcAttachType not a field of TcOptions too? Seems pretty arbitrary.

I still think that if we don't want to add a separate method, #418 (comment) is still the best option: the types are self describing and if you don't care about them you can still pass 0 as a default value which is less tedious than typing Blah::default() (which still remains an option).

If we had more options, I think my preference would be fn attach(iface: &str, attach_type: TcAttachType) as it is today, and then add a separate attach_with_options(iface: &str, attach_type: TcAttachType, opts: TcOptions), but having a separate method and a separate struct for two options seems a bit unnecessary.

anfredette · 2022-11-04T18:47:55Z

@alessandrod, Thanks for your comments.

I don't think bpf programs can be attached to anything other than clsact?

I had to do some experimentation with tc before I could respond because I wasn't positive myself.

bpf programs can still be attached to other qdiscs. As an example, let’s say we have a PRIO qdisc with multiple classes. We could use standard tc filters to classify packets into classes. However, a bpf filter would allow more flexible classification. Additionally, this could be a non-direct-action program that returns a class. For example, the following tc commands could be used to create that qdisc and then attach a bpf filter that reclassifies icmp packets:

sudo tc qdisc add dev eth0 root handle 1: prio
sudo tc qdisc add dev eth0 parent 1:1 handle 10: sfq
sudo tc qdisc add dev eth0 parent 1:2 handle 20: sfq
sudo tc qdisc add dev eth0 parent 1:3 handle 30: sfq
sudo tc filter add dev eth0 parent 1: bpf obj classify-icmp.o sec tc

The ingress qdisc also still works, but given that clsact is a generalization of that qdisc, I don’t see a reason why the clsact qdisc couldn’t be used instead.

tc-bpf also supports other options including skip_hw/skip_sw, police, and export that seem like they could be useful.

Why is TcAttachType not a field of TcOptions too? Seems pretty arbitrary.

That’s a good point. If we went with the TcOptions approach, TcAttachType could be an option as well. The tc-bpf cli defaults to egress, so that would be a reasonable default for us as well.

This API seems pretty un-rusty to me

I’m fairly new to Rust, so I’ll let you be the judge on whether this is rusty enough :-).

We don’t currently have requirements for these other options or use cases yet, so I’m not suggesting we support any right now, but we were thinking that the proposal supports easily extending the API for more options in the future without disrupting existing code. However, if you don’t like my last proposal, I’m fine with either of your suggestions.

anfredette · 2022-11-08T14:27:19Z

@alessandrod, I'm checking in again on this. In my last comment, I expanded a bit on why I think there's potential for the support of additional options in the future. However, I'm also okay with either of your suggestions. Thanks, Andre

alessandrod · 2022-11-09T00:08:48Z

If we had more options, I think my preference would be fn attach(iface: &str, attach_type: TcAttachType) as it is today, and then add a separate attach_with_options(iface: &str, attach_type: TcAttachType, opts: TcOptions)

I think we should go for this. It makes the common case as easy as possible, and it makes the more complex cases future proof.

anfredette · 2022-11-09T15:41:14Z

If we had more options, I think my preference would be fn attach(iface: &str, attach_type: TcAttachType) as it is today, and then add a separate attach_with_options(iface: &str, attach_type: TcAttachType, opts: TcOptions)

I think we should go for this. It makes the common case as easy as possible, and it makes the more complex cases future proof.

Sounds good. Thanks.

Signed-off-by: Andre Fredette <afredette@redhat.com>

anfredette · 2022-11-09T19:41:22Z

The API has been updated as requested.

anfredette · 2022-11-16T11:59:29Z

@alessandrod, this should be ready to go, so please take a look when you have a chance. Thanks, Andre

alessandrod

Almost there, just some doc fixes :)

alessandrod · 2022-11-16T21:59:44Z

aya/src/programs/tc.rs

-/// prog.attach("eth0", TcAttachType::Ingress, 0)?;
+///
+///  // the following demonstrates using the standard attach with default options
+/// prog.attach("eth0", TcAttachType::Ingress)?;


we should only show the simple case here. The attach_with_options example should go in the rustdocs of that method

alessandrod · 2022-11-16T22:02:57Z

aya/src/programs/tc.rs

+/// Options for SchedClassifier attach
+#[derive(Default)]
+pub struct TcOptions {
+    /// `priority`: priority assigned to tc program with lower number = higher priority.


No need to repeat the field name, see https://deploy-preview-418--aya-rs-docs.netlify.app/user/aya/programs/tc/struct.tcoptions (the first comment in the PR has a preview of docs including your changes, which is super helpful to check that everything looks ok)

/// The priority the program is attached with. /// /// Lower value means higher priority. If set to `0` the system chooses the next /// highest priority or `0xC000` when attaching the first filter. pub priority: u16, /// Unique identifier for a program at a given priority level. If set to `0`, /// the system chooses a handle. pub handle: u32,

Signed-off-by: Andre Fredette <afredette@redhat.com>

alessandrod · 2022-11-18T06:36:04Z

Thank you for working on this Andre!

anfredette · 2022-11-18T14:40:55Z

Thank you for working on this Andre!

You're welcome. Thanks @alessandrod and @vadorovsky for reviewing it!

vadorovsky reviewed Oct 18, 2022

View reviewed changes

aya/src/programs/tc.rs Outdated Show resolved Hide resolved

aya/src/sys/netlink.rs Outdated Show resolved Hide resolved

anfredette force-pushed the tc-handle branch from b7a011b to 5baa671 Compare October 19, 2022 13:46

vadorovsky approved these changes Oct 19, 2022

View reviewed changes

alessandrod reviewed Oct 20, 2022

View reviewed changes

anfredette force-pushed the tc-handle branch from 5baa671 to ac07608 Compare October 24, 2022 17:24

Support both attach() and attach_with_options() for SchedClassifier

a3e3e80

Signed-off-by: Andre Fredette <afredette@redhat.com>

anfredette force-pushed the tc-handle branch from 519781d to a3e3e80 Compare November 9, 2022 19:01

alessandrod reviewed Nov 16, 2022

View reviewed changes

Make doc fixes

abb75ba

Signed-off-by: Andre Fredette <afredette@redhat.com>

alessandrod approved these changes Nov 18, 2022

View reviewed changes

alessandrod merged commit 7fef833 into aya-rs:main Nov 18, 2022

anfredette deleted the tc-handle branch November 18, 2022 14:39

dave-tucker added the feature A PR that implements a new feature or enhancement label Feb 23, 2023

dave-tucker added the aya This is about aya (userspace) label Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support using handle in tc programs #418

Support using handle in tc programs #418

anfredette commented Oct 17, 2022

netlify bot commented Oct 17, 2022 •

edited

Loading

vadorovsky left a comment

anfredette commented Oct 19, 2022

dave-tucker commented Oct 20, 2022

alessandrod Oct 20, 2022

anfredette Oct 20, 2022

alessandrod Oct 20, 2022 •

edited

Loading

alessandrod Oct 20, 2022

alessandrod Oct 20, 2022

anfredette Oct 20, 2022

anfredette Oct 20, 2022

anfredette commented Oct 24, 2022 •

edited

Loading

anfredette commented Oct 24, 2022

dave-tucker commented Nov 2, 2022

alessandrod commented Nov 2, 2022

anfredette commented Nov 4, 2022

anfredette commented Nov 8, 2022

alessandrod commented Nov 9, 2022

anfredette commented Nov 9, 2022

anfredette commented Nov 9, 2022

anfredette commented Nov 16, 2022

alessandrod left a comment

alessandrod Nov 16, 2022

anfredette Nov 17, 2022

alessandrod Nov 16, 2022

alessandrod Nov 16, 2022

anfredette Nov 17, 2022

alessandrod commented Nov 18, 2022

anfredette commented Nov 18, 2022

		@@ -124,19 +127,28 @@ impl SchedClassifier {
		interface: &str,

Support using handle in tc programs #418

Support using handle in tc programs #418

Conversation

anfredette commented Oct 17, 2022

netlify bot commented Oct 17, 2022 • edited Loading

✅ Deploy Preview for aya-rs-docs ready!

vadorovsky left a comment

Choose a reason for hiding this comment

anfredette commented Oct 19, 2022

dave-tucker commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandrod Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anfredette commented Oct 24, 2022 • edited Loading

anfredette commented Oct 24, 2022

dave-tucker commented Nov 2, 2022

alessandrod commented Nov 2, 2022

anfredette commented Nov 4, 2022

anfredette commented Nov 8, 2022

alessandrod commented Nov 9, 2022

anfredette commented Nov 9, 2022

anfredette commented Nov 9, 2022

anfredette commented Nov 16, 2022

alessandrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandrod commented Nov 18, 2022

anfredette commented Nov 18, 2022

netlify bot commented Oct 17, 2022 •

edited

Loading

alessandrod Oct 20, 2022 •

edited

Loading

anfredette commented Oct 24, 2022 •

edited

Loading