Add Support for TCX Programs #1222

anfredette · 2024-08-12T16:51:14Z

This PR adds support for TCX programs using priority-based API for
TCX modeled after the existing TC API.

Both the Linux kernel and Aya use a relative positioning approach for
ordering TCX programs that requires new TCX programs to be positioned
relative to an existing TCX program. To improve the user experience
across multiple independent applications, we’ve retained a TC-like
priority API in bpfman, which internally maps to Aya’s relative
ordering API.

Additional Changes:

A TCX integration test is included.
A go-tcx-counter example was added, and the go-app-counter example has been
updated to include a TCX program.
GitHub Actions are now configured to use ubuntu-24.04 to meet the kernel
version requirement (6.6 or higher) for TCX.

Fixes: #1215

codecov · 2024-08-12T20:34:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 8.57%. Comparing base (0b69a8d) to head (82a6937).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##            main   #1222      +/-   ##
========================================
- Coverage   8.94%   8.57%   -0.37%     
========================================
  Files         24      24              
  Lines       6027    6285     +258     
========================================
  Hits         539     539              
- Misses      5488    5746     +258

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mergify · 2024-08-30T13:13:11Z

@anfredette, this pull request is now in conflict and requires a rebase.

anfredette · 2024-10-11T14:45:15Z

This TCX pr is ready for review.

msherif1234 · 2024-10-14T16:23:53Z

bpfman-api/src/bin/rpc/rpc.rs

+            Info::TcxAttachInfo(TcxAttachInfo {
+                priority,
+                iface,
+                position: _,


can u drop position if its not in use ?

position is set later when the program is attached.

msherif1234 · 2024-10-14T16:24:30Z

bpfman-api/src/bpfman.v1.rs

+    #[prost(string, tag = "2")]
+    pub iface: ::prost::alloc::string::String,
+    #[prost(int32, tag = "3")]
+    pub position: i32,


same here ?

position is set later when the program is attached.

msherif1234 · 2024-10-14T16:28:17Z

bpfman/src/lib.rs

+        links::FdLink,
+        loaded_programs,
+        tc::{SchedClassifierLink, SchedClassifierLinkId, TcAttachOptions},
+        trace_point::TracePointLink,


where is tcx in this ?

A tcx and tc programs are considered to be the same type by Aya and the kernel. It's the attach type that's different.

msherif1234 · 2024-10-14T16:29:36Z

bpfman/src/lib.rs

@@ -737,6 +762,105 @@ fn filter(
        })
 }

+fn get_txc_programs(root_db: &Db, if_index: u32, direction: Direction) -> Vec<TcxProgram> {


typo ? should be tcx ?

Billy99 · 2024-10-14T17:43:33Z

bpfman/src/lib.rs

+            let id = bytes_to_string(&p)
+                .split('_')
+                .last()
+                .unwrap()


I would like to remove the unwrap() calls. This pattern is in this file a few times. Move it to a separate function and re-code it without the unwrap() calls.

I added a helper function to get the id and fixed this one instance. However, it requires returning a Result in this function and handling it several other places. If people are okay with the way I did it, then I can modify the existing code similarly.

Billy99 · 2024-10-14T17:44:47Z

bpfman/src/lib.rs

+        if order.is_none() {
+            np.set_current_position(tcx_programs.len())?;
+            order = Some(AttachOrder::After(
+                tcx_programs.last().unwrap().get_data().get_id()?,


Can this unwrap() fail? Would prefer not to have it if possible.

Actually, this one cannot fail because I've already checked tcx_programs.is_empty() at the top, and if it was it would have already returned. However, I plan to split this function into two separate functions to handle the cases with and without a new_program. I'd also like to see if I can make this whole function cleaner. I'll handle the unwrap() when I do that.

I re-worked this function and updated the last commit.

Billy99 · 2024-10-14T17:46:52Z

bpfman/src/lib.rs

+            let owned_link: SchedClassifierLink = tcx.take_link(link_id)?;
+            let fd_link: FdLink = owned_link
+                .try_into()
+                .expect("unable to get owned tcx attach link");


.expect() calls are scattered throughout the code, but if they are hit, they are a panic. I would like to avoid adding any new ones and slowly remove all the existing ones in the code.

Billy99 · 2024-10-14T17:48:16Z

bpfman/src/types.rs

+
+    pub fn get_direction(&self) -> Result<Direction, BpfmanError> {
+        sled_get(&self.data.db_tree, TCX_DIRECTION)
+            .map(|v| bytes_to_string(&v).to_string().try_into().unwrap())


Avoid the unwrap().

Billy99 · 2024-10-14T17:49:24Z

bpfman/src/types.rs

+                // result, we use the following hack to figure out which one it
+                // really is.
+                ProgramType::Tc => {
+                    if data.db_tree.get(TCX_IFACE).unwrap().is_some() {


Avoid the unwrap().

Billy99 · 2024-10-14T19:03:06Z

examples/go-tcx-counter/container-deployment/Containerfile.go-tcx-counter

+RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build
+
+
+FROM --platform=$TARGETPLATFORM registry.fedoraproject.org/fedora-minimal:latest


Suggested change

FROM --platform=$TARGETPLATFORM registry.fedoraproject.org/fedora-minimal:latest

FROM registry.fedoraproject.org/fedora-minimal:latest

Removed in all the files in the multi-arch PR.

Billy99 · 2024-10-14T19:05:21Z

examples/go-tcx-counter/container-deployment/Containerfile.go-tcx-counter

+ARG BUILDPLATFORM
+
+# The following ARGs are set internally by docker or podman on multiarch builds
+ARG TARGETPLATFORM=linux/amd64


Suggested change

ARG TARGETPLATFORM=linux/amd64

ARG TARGETARCH

ARG TARGETOS

ARG TARGETPLATFORM

RUN echo "TARGETOS=${TARGETOS} TARGETARCH=${TARGETARCH} BUILDPLATFORM=${BUILDPLATFORM} TARGETPLATFORM=${TARGETPLATFORM}"

Similar change made in all the files in the multi-arch PR.

Billy99 · 2024-10-14T19:06:58Z

examples/go-tcx-counter/container-deployment/Containerfile.go-tcx-counter

+
+FROM --platform=$TARGETPLATFORM registry.fedoraproject.org/fedora-minimal:latest
+
+ARG TARGETPLATFORM


Suggested change

ARG TARGETPLATFORM

Billy99 · 2024-10-14T19:17:58Z

bpfman/src/lib.rs

+                .unwrap()
+                .parse::<u32>()
+                .unwrap();
+            let tree = root_db.open_tree(p).expect("unable to open database tree");


.expect() calls are scattered throughout the code, but if they are hit, they are a panic. I would like to avoid adding any new ones and slowly remove all the existing ones in the code.

Billy99 · 2024-10-14T19:20:26Z

bpfman/src/lib.rs

@@ -215,6 +224,10 @@ pub async fn add_program(mut program: Program) -> Result<Program, BpfmanError> {

            add_multi_attach_program(root_db, &mut program, &mut image_manager, config).await
        }
+        Program::Tcx(_) => {
+            program.set_if_index(get_ifindex(&program.if_name().unwrap())?)?;


Can this unwrap() fail? Would prefer not to have it if possible.

mergify · 2024-10-16T12:55:19Z

@anfredette, this pull request is now in conflict and requires a rebase.

bpfman/src/utils.rs

mergify · 2024-10-21T13:52:37Z

@anfredette, this pull request is now in conflict and requires a rebase.

This PR adds support for TCX programs using priority-based API for TCX modeled after the existing TC API. Both the Linux kernel and Aya use a relative positioning approach for ordering TCX programs that requires new TCX programs to be positioned relative to an existing TCX program. To improve the user experience across multiple independent applications, we’ve retained a TC-like priority API in bpfman, which internally maps to Aya’s relative ordering API. Additional Changes: - A TCX integration test is included. - A go-tcx-counter example was added, and the go-app-counter example has been updated to include a TCX program. - GitHub Actions are now configured to use ubuntu-24.04 to meet the kernel version requirement (6.6 or higher) for TCX. Signed-off-by: Andre Fredette <[email protected]>

Also split up and reworked the update_tcx_program_positions() function. Signed-off-by: Andre Fredette <[email protected]>

msherif1234 · 2024-10-21T16:09:01Z

/LGTM

Billy99

/lgtm

Before I approve, quick question. How does bpfman handle it if TCX programs are already installed on the system outside of bpfman? For TC and XDP, it doesn't, but since the multi-prog is handled by the kernel, what is the expect behavior?

anfredette · 2024-10-21T20:41:45Z

Both bpfman and other applications can install tcx programs. We don't currently pay attention to TCX programs installed by other means. The first bpfman TCX program on a given attach point is placed in first position regardless of whether there are already other programs attached. Subsequent bpfman TCX programs on the same attach point are positioned relative to existing bpfman programs based on priority without regard to any other TCX programs that may have been attached by other means. We've talked about some options for cooperating better with non-bpfman loaders, but this is the first step.

anfredette force-pushed the tcx-support branch from de1d0bf to 1e5753d Compare August 12, 2024 16:53

anfredette changed the title ~~bpfman TCX support~~ DRAFT bpfman TCX support Aug 12, 2024

anfredette changed the title ~~DRAFT bpfman TCX support~~ Draft bpfman TCX support Aug 12, 2024

anfredette force-pushed the tcx-support branch 3 times, most recently from 81c8ff9 to ef40d84 Compare August 12, 2024 20:21

anfredette force-pushed the tcx-support branch 2 times, most recently from ebc8772 to f093d59 Compare August 13, 2024 13:46

anfredette mentioned this pull request Aug 13, 2024

Change image for basic-integration-tests back to "ubuntu-latest" after it supports tcx #1224

Open

msherif1234 mentioned this pull request Aug 14, 2024

TCX ebpf hook support bpfman/bpfman-operator#102

Merged

anfredette force-pushed the tcx-support branch 10 times, most recently from a058491 to 6d03cd7 Compare August 26, 2024 22:10

anfredette force-pushed the tcx-support branch 3 times, most recently from e254539 to 247f0f3 Compare August 28, 2024 17:29

anfredette changed the title ~~Draft bpfman TCX support~~ Add Support for TCX Programs Aug 28, 2024

mergify bot added the needs-rebase label Aug 30, 2024

anfredette force-pushed the tcx-support branch from 247f0f3 to b4f30a0 Compare September 6, 2024 18:46

mergify bot removed the needs-rebase label Sep 6, 2024

anfredette force-pushed the tcx-support branch from b4f30a0 to e820a23 Compare September 26, 2024 16:14

mergify bot added the needs-rebase label Oct 9, 2024

anfredette force-pushed the tcx-support branch from e7b6e8b to b4dc28b Compare October 11, 2024 14:15

mergify bot removed the needs-rebase label Oct 11, 2024

anfredette marked this pull request as ready for review October 11, 2024 14:44

anfredette requested a review from a team as a code owner October 11, 2024 14:44

msherif1234 reviewed Oct 14, 2024

View reviewed changes

Billy99 reviewed Oct 14, 2024

View reviewed changes

mergify bot added the needs-rebase label Oct 16, 2024

anfredette force-pushed the tcx-support branch from b4dc28b to e1fba01 Compare October 19, 2024 22:02

mergify bot removed the needs-rebase label Oct 19, 2024

anfredette force-pushed the tcx-support branch from e1fba01 to 4719179 Compare October 20, 2024 12:26

github-advanced-security bot found potential problems Oct 20, 2024

View reviewed changes

bpfman/src/utils.rs Fixed Show fixed Hide fixed

anfredette force-pushed the tcx-support branch 2 times, most recently from 83c36cb to bfa64cf Compare October 21, 2024 12:36

mergify bot added the needs-rebase label Oct 21, 2024

anfredette added 2 commits October 21, 2024 11:41

Address review comments from Billy and Mohammed

82a6937

Also split up and reworked the update_tcx_program_positions() function. Signed-off-by: Andre Fredette <[email protected]>

anfredette force-pushed the tcx-support branch from bfa64cf to 82a6937 Compare October 21, 2024 15:49

mergify bot removed the needs-rebase label Oct 21, 2024

Billy99 reviewed Oct 21, 2024

View reviewed changes

Billy99 approved these changes Oct 21, 2024

View reviewed changes

mergify bot merged commit 6c1a8e6 into bpfman:main Oct 21, 2024
56 checks passed

		RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build


		FROM --platform=$TARGETPLATFORM registry.fedoraproject.org/fedora-minimal:latest

	FROM --platform=$TARGETPLATFORM registry.fedoraproject.org/fedora-minimal:latest
	FROM registry.fedoraproject.org/fedora-minimal:latest

-ARG TARGETPLATFORM=linux/amd64
+ARG TARGETARCH
+ARG TARGETOS
+ARG TARGETPLATFORM
+RUN echo "TARGETOS=${TARGETOS}  TARGETARCH=${TARGETARCH}  BUILDPLATFORM=${BUILDPLATFORM}  TARGETPLATFORM=${TARGETPLATFORM}"


		FROM --platform=$TARGETPLATFORM registry.fedoraproject.org/fedora-minimal:latest

		ARG TARGETPLATFORM

Add Support for TCX Programs #1222

Add Support for TCX Programs #1222

Conversation

anfredette commented Aug 12, 2024 • edited Loading

codecov bot commented Aug 12, 2024 • edited Loading

Codecov Report

mergify bot commented Aug 30, 2024

anfredette commented Oct 11, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anfredette Oct 20, 2024 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Oct 16, 2024

mergify bot commented Oct 21, 2024

msherif1234 commented Oct 21, 2024

Billy99 left a comment

Choose a reason for hiding this comment

anfredette commented Oct 21, 2024

anfredette commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

anfredette Oct 20, 2024 •

edited

Loading