Allow users to ignore links when enabling XDP #25870

poblahblahblah · 2023-06-02T16:29:59Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
[?] All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
[?] Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

This allows a user to specify a regex that will cause a link to be ignored when enabling XDP. We need this ability since we are running physical hosts where the primary NICs do support XDP, but have additional vlan devices that do not have XDP support in the driver.

More background can be found in #24768, but to summarize we have 1 physical device (ens785np0) per host and due to network design we need an additional vlan type interface (ens785np0.3). Regrettably the vlan driver in the kernel does not support XDP. When enabling XDP without this change Cilium enters a crashloop with this error:

Failed to compile XDP program" error="program cil_xdp_entry: attaching XDP program to interface ens785np0.3: operation not supported" subsys=datapath-loader

With this change applied and by specifying xdp-ignore-device-name-regex in the Cilium config we now get XDP enabled on the device and tc for the vlan device:

$ sudo bpftool net
xdp:
ens785np0(2) driver id 22899

tc:
ens785np0(2) clsact/ingress cil_from_netdev-ens785np0 id 23063
ens785np0(2) clsact/egress cil_to_netdev-ens785np0 id 23077
cilium_net(4) clsact/ingress cil_to_host-cilium_net id 23052
cilium_host(5) clsact/ingress cil_to_host-cilium_host id 23005
cilium_host(5) clsact/egress cil_from_host-cilium_host id 23051
cilium_vxlan(6) clsact/ingress cil_from_overlay-cilium_vxlan id 22907
cilium_vxlan(6) clsact/egress cil_to_overlay-cilium_vxlan id 22903
lxc4ecafafd804a(10) clsact/ingress cil_from_container-lxc4ecafafd804a id 23022
lxc9e4006f5d109(12) clsact/ingress cil_from_container-lxc9e4006f5d109 id 22950
lxcb6797ab24b31(14) clsact/ingress cil_from_container-lxcb6797ab24b31 id 22914
lxc22d432e999bb(16) clsact/ingress cil_from_container-lxc22d432e999bb id 22913
lxcecd9c72cbc66(18) clsact/ingress cil_from_container-lxcecd9c72cbc66 id 22983
lxc123aaba50ec5(20) clsact/ingress cil_from_container-lxc123aaba50ec5 id 22937
lxc602295b67e63(22) clsact/ingress cil_from_container-lxc602295b67e63 id 23028
lxcd5f791798257(24) clsact/ingress cil_from_container-lxcd5f791798257 id 22917
lxc3bd4bca11864(26) clsact/ingress cil_from_container-lxc3bd4bca11864 id 23017
lxc1ecd38cdf625(28) clsact/ingress cil_from_container-lxc1ecd38cdf625 id 23039
lxcb36787663030(30) clsact/ingress cil_from_container-lxcb36787663030 id 22922
ens785np0.3(57) clsact/ingress cil_from_netdev-ens785np0.3 id 23090
ens785np0.3(57) clsact/egress cil_to_netdev-ens785np0.3 id 23103
lxc_health(79) clsact/ingress cil_from_container-lxc_health id 23008

flow_dissector:

@borkmann indicated in #24768 (comment) that a regex to filter out a device name would be acceptable.

Happy to make any adjustments and I am looking forward to getting this merged!

Note: I have not yet added support for this option to the Helm chart. Happy to do so in this PR or in a follow up if desired. I am holding off on it just to ensure that the variable names I chose are acceptable.

Fixes: #24768

Adds ability to filter out device names via regex when enabling XDP

squeed · 2023-06-06T09:38:33Z

Out of curiosity, should the vlan devices be completely ignored, or just for the sake of XDP?

pkg/datapath/loader/base.go

squeed · 2023-06-06T09:50:39Z

Question for you, @borkmann -- given the usual XDP use-case, would an allowing regex make more sense (i.e. use XDP for devices matching the regex)? Or should we stick with an excluding regex?

Separately, are there any other specifiers that might make sense? Only interfaces with a given driver?

pkg/datapath/loader/base.go

poblahblahblah · 2023-06-06T21:02:16Z

Out of curiosity, should the vlan devices be completely ignored, or just for the sake of XDP?

I'm not positive!

I think regardless of whether it's beneficial to use tc on a vlan type device the change would be useful for other groups who want to have XDP but have mixed NICs where one supports XDP and the other does not.

thorn3r

changes lgtm, pending the question @squeed raised about inverting the option. Mind squashing the commits?

pkg/datapath/loader/base.go

poblahblahblah · 2023-06-08T20:48:01Z

Would you all like me to update the Helm chart and docs while I am in here?

poblahblahblah · 2023-06-13T15:34:27Z

Hello, is there anything else you would like to see for this change?

poblahblahblah · 2023-06-20T17:55:45Z

Hello!

Wondering if there's anything I can do to help move this change along.

Let me know!

rgo3

Apologies for the wait @poblahblahblah. We should still clarify some of the questions by @squeed above, but a release is coming up so a lot of folks are stretched a bit thin. Apart from the two code style nits, this LGTM so I'll approve.

daemon/cmd/daemon_main.go

pkg/datapath/loader/base.go

poblahblahblah · 2023-08-18T02:09:02Z

Rebased!

ti-mo

Thanks, left a few nits.

pkg/datapath/loader/base.go

daemon/cmd/daemon_main.go

poblahblahblah · 2023-08-18T22:04:57Z

@ti-mo updated per your recommendations

pkg/datapath/loader/base.go

daemon/cmd/daemon_main.go

This allows a user to specify a regex that will cause a link to be ignored when enabling XDP. We need this ability since we are running physical hosts where the primary NICs do support XDP, but have additional `vlan` devices that _do not_ have XDP support in the driver. Fixes: cilium#24768 Signed-off-by: Patrick O’Brien <patrick.obrien@thetradedesk.com>

poblahblahblah · 2023-08-23T15:28:15Z

@ti-mo all suggested changes have been made. I also rebased against the latest main

Thanks for the input - I appreciate the opportunity to improve!

ti-mo

Looks good! Thanks for addressing all of my feedback. 🙏

ti-mo · 2023-08-24T08:12:17Z

/test

poblahblahblah · 2023-08-24T15:22:30Z

It looks like a single test failed - can that test be retried?

joestringer · 2023-08-24T18:44:00Z

daemon/cmd/daemon_main.go

@@ -1543,6 +1543,13 @@ func initEnv(vp *viper.Viper) {
 			)
 		}
 	}
+
+	// Ensure the xdp-ignore-device-regex compiles
+	if len(option.Config.XDPIgnoreDeviceNameRegex) > 0 {


Was this question from @squeed ever discussed? Scanning through I could see a few folks saying there should be a discussion but I couldn't see a clear resolution to the question:

Question for you, @borkmann -- given the usual XDP use-case, would an allowing regex make more sense (i.e. use XDP for devices matching the regex)? Or should we stick with an excluding regex?

Separately, are there any other specifiers that might make sense? Only interfaces with a given driver?

Sorry for the huge delay on my side. First release, and then out for long PTO, it slipped my radar.

Just thinking some more on this topic, @poblahblahblah / @joestringer / @joamaki, what if instead of regex we just had a simple bool option where we say "if XDP attach fails on this device, then just fallback to tc BPF". Would this be more generic and easier to use rather than specifying device regex? Something like --bpf-lb-acceleration-fallback=true [ In case BPF load balancing acceleration via native XDP is not supported for some devices, fall back to tc BPF attachment ]. Other option could be to have a new mode --bpf-lb-acceleration=best-effort ?

Makes sense to me, no need for the config flag even IMO. If the device is expected to be managed, we should make it so.

You mean just to fallback in case of failure with the native option? I think this perhaps could lead to false conclusions, meaning people will try to active, then some ethtool driver channel setting is not correct and it'll fail and people just assume XDP would be there now but actually we run in tc BPF mode. So I would bail out by default and let users make a conscious decision to fallback upon failure. Also what would help as well in this context would be to let cilium status --verbose dump the devices which have Cilium progs on XDP so that this is easily visible for troubleshooting / and sysdump.

So you are proposing the following:

Keep the default behavior of bailing out in place

Implement --bpf-lb-acceleration=best-effort which if set would have devices fall back to TC

If I updated this PR to do the above, would the PR be considered mergeable?

If I updated this PR to do the above, would the PR be considered mergeable?

Yes, definitely - if code looks good then I consider it mergable.

@borkmann Sorry for the delay. I made a first pass at this change here: #28666

poblahblahblah · 2023-08-28T17:31:24Z

I would love to see this MR merged so we can enable XDP in our environment!

Is there anything I can do to help move this along further?

lmb · 2023-09-25T14:03:59Z

Note to future triagers: this is waiting for #25870 (comment) to be implemented.

ti-mo · 2023-10-04T11:41:09Z

@lmb Small note: we can convert PRs to drafts if they need a course change.

@poblahblahblah Please mark ready for review if this is ready for another round.

poblahblahblah · 2023-11-16T22:26:01Z

Closing in favor of #28666

poblahblahblah requested review from a team as code owners June 2, 2023 16:29

poblahblahblah requested review from thorn3r, squeed and rgo3 June 2, 2023 16:30

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 2, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jun 2, 2023

poblahblahblah marked this pull request as draft June 2, 2023 16:42

poblahblahblah force-pushed the add-xdp-ignore-list branch from d9e433e to 9c3aa70 Compare June 2, 2023 17:03

poblahblahblah marked this pull request as ready for review June 2, 2023 17:04

squeed reviewed Jun 6, 2023

View reviewed changes

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved

rgo3 reviewed Jun 6, 2023

View reviewed changes

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved

thorn3r approved these changes Jun 8, 2023

View reviewed changes

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved

poblahblahblah force-pushed the add-xdp-ignore-list branch from f0f5cb8 to b1ca84a Compare June 8, 2023 21:11

poblahblahblah requested review from a team as code owners June 8, 2023 21:11

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 20, 2023

rgo3 approved these changes Jun 23, 2023

View reviewed changes

daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved

rgo3 added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 23, 2023

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 18, 2023

poblahblahblah force-pushed the add-xdp-ignore-list branch from dc4b2a4 to 2f97fb2 Compare August 18, 2023 02:08

ti-mo requested changes Aug 18, 2023

View reviewed changes

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved

pkg/datapath/loader/base.go Show resolved Hide resolved

daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved

ti-mo removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 18, 2023

poblahblahblah force-pushed the add-xdp-ignore-list branch from 2f97fb2 to 618ab40 Compare August 18, 2023 22:04

poblahblahblah force-pushed the add-xdp-ignore-list branch from 618ab40 to 73307cd Compare August 18, 2023 22:05

ti-mo requested changes Aug 21, 2023

View reviewed changes

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved

ti-mo reviewed Aug 23, 2023

View reviewed changes

daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved

poblahblahblah force-pushed the add-xdp-ignore-list branch from 73307cd to 3b9a40c Compare August 23, 2023 15:27

ti-mo approved these changes Aug 24, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 24, 2023

joestringer reviewed Aug 24, 2023

View reviewed changes

joestringer requested a review from borkmann August 24, 2023 18:44

joestringer added dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 24, 2023

ti-mo marked this pull request as draft October 4, 2023 11:41

poblahblahblah mentioned this pull request Oct 18, 2023

Best effort for xdp #28666

Merged

6 tasks

poblahblahblah closed this Nov 16, 2023

poblahblahblah deleted the add-xdp-ignore-list branch November 16, 2023 22:26

qmonnet mentioned this pull request Jul 16, 2024

XDP: Failing to attach XDP program to a tagged VLAN interface #24768

Closed

2 tasks

Allow users to ignore links when enabling XDP #25870

Allow users to ignore links when enabling XDP #25870

Uh oh!

Conversation

poblahblahblah commented Jun 2, 2023

Uh oh!

squeed commented Jun 6, 2023

Uh oh!

Uh oh!

squeed commented Jun 6, 2023

Uh oh!

Uh oh!

poblahblahblah commented Jun 6, 2023

Uh oh!

thorn3r left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

poblahblahblah commented Jun 8, 2023

Uh oh!

poblahblahblah commented Jun 13, 2023

Uh oh!

poblahblahblah commented Jun 20, 2023

Uh oh!

rgo3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

poblahblahblah commented Aug 18, 2023

Uh oh!

ti-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poblahblahblah commented Aug 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poblahblahblah commented Aug 23, 2023

Uh oh!

ti-mo left a comment

Choose a reason for hiding this comment

Uh oh!

ti-mo commented Aug 24, 2023

Uh oh!

poblahblahblah commented Aug 24, 2023

Uh oh!

joestringer Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

borkmann Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

ti-mo Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

borkmann Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poblahblahblah Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

borkmann Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

poblahblahblah Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

poblahblahblah commented Aug 28, 2023

Uh oh!

lmb commented Sep 25, 2023

Uh oh!

ti-mo commented Oct 4, 2023

Uh oh!

poblahblahblah commented Nov 16, 2023

borkmann Aug 31, 2023 •

edited

Loading