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

GCC BPF support #164

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

GCC BPF support #164

wants to merge 2 commits into from

Conversation

theihor
Copy link
Contributor

@theihor theihor commented Dec 27, 2024

Successor of #144

@theihor theihor force-pushed the bpf-gcc branch 2 times, most recently from 6922229 to 83066ab Compare December 28, 2024 00:04
theihor added a commit to theihor/vmtest that referenced this pull request Dec 28, 2024
@theihor theihor force-pushed the bpf-gcc branch 3 times, most recently from 835e83e to ed0eb19 Compare January 10, 2025 20:40
@theihor theihor changed the title Add build-bpf-gcc action GCC BPF support Jan 10, 2025
@theihor
Copy link
Contributor Author

theihor commented Jan 10, 2025

@chantra this one makes GCC BPF specific changes. It should be reviewed/merged after #166

@theihor theihor requested a review from chantra January 10, 2025 20:42
This was referenced Jan 10, 2025
theihor added a commit to theihor/vmtest that referenced this pull request Jan 10, 2025
theihor added a commit to theihor/vmtest that referenced this pull request Jan 10, 2025
@theihor theihor force-pushed the bpf-gcc branch 4 times, most recently from 84b0a59 to a11be94 Compare January 14, 2025 19:25
A prerequisite for running selftests/bpf is a recent version of GCC
compiled for BPF backend.

Signed-off-by: Ihor Solodrai <[email protected]>
When BUILD_BPF_GCC is set in kernel-build, GCC BPF will be built and
BPF_GCC variable set for build-selftests. When BPF_GCC is set,
test_progs-bpf_gcc runner is built in selftests/bpf.

If test_progs-bpf_gcc is specified in KERNEL_TEST, it will be executed
by run-vmtest action.

Signed-off-by: Ihor Solodrai <[email protected]>
Copy link
Collaborator

@chantra chantra left a comment

Choose a reason for hiding this comment

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

LGTM, I am just not sure if the BPF-GCC is complementary to the current bpf build, e.g after this build, we have 2 distinctive bpf objects, 1 for the llvm build, and one for the gcc one?

BPF_NEXT_BASE_BRANCH: 'master'
BPF_NEXT_FETCH_DEPTH: 64 # A bit of history is needed to facilitate incremental builds
BUILD_SCHED_EXT_SELFTESTS: ${{ inputs.arch == 'x86_64' || inputs.arch == 'aarch64' && 'true' || '' }}
# BUILD_SCHED_EXT_SELFTESTS: ${{ inputs.arch == 'x86_64' || inputs.arch == 'aarch64' && 'true' || '' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this related to this diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'll clean it up, thanks.

Comment on lines +49 to +50

BUILD_BPF_GCC: ${{ inputs.arch == 'x86_64' && 'true' || '' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this going to build bpf-gcc for all combination of x86 builds?

shouldn't we instead define a "bpf-toolchain" that defaults to llvm (current behaviour), and can potentially be changd to gcc and use this build? Or is this handled transparently by the "test_progs-bpf_gcc" target?

Copy link
Contributor Author

@theihor theihor Jan 14, 2025

Choose a reason for hiding this comment

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

Right. This is going to build GCC for all x86 builds, but since it is cached, it's not an issue. When cached, it gets pulled in about 6 seconds: https://github.com/libbpf/ci/actions/runs/12777030966/job/35616953725

I thought about making a separate job (a dependency of kernel-test, when BUILD_BPF_GCC is set), since GCC doesn't really depend on anything within our workflows. But I concluded it won't make a difference, because the tests still depend on kernel to be built.

@theihor
Copy link
Contributor Author

theihor commented Jan 14, 2025

@chantra please see an explanation re these two questions below

I am just not sure if the BPF-GCC is complementary to the current bpf build, e.g after this build, we have 2 distinctive bpf objects, 1 for the llvm build, and one for the gcc one?

shouldn't we instead define a "bpf-toolchain" that defaults to llvm (current behaviour), and can potentially be changd to gcc and use this build? Or is this handled transparently by the "test_progs-bpf_gcc" target?

Yes, it's handled transparently in selftests/bpf/Makefile.

The "bpf-toolchain" doesn't really change when we set BPF_GCC. This is why I decided to not include the changes from #144 that add "bpf-toolchain" action arguments.

For example, let's say the CI toolchain-full is set to "llvm-17". So, the kernel is built with LLVM=-17 (see here), and the test_progs binaries are built with llvm-17 (not the default).

Now, on x86 BUILD_BPF_GCC is true, so BPF_GCC will be set to where freshly built gcc binaries are located. The test_progs-bpf_gcc binary is still built with clang 17 in this case (make target just uses $(CC)). What changes is:

  1. test_progs-bpf_gcc is built (by default it's not)
  2. The BPF objects for test_progs-bpf_gcc are built with provided GCC compiler. Later they are automagically included in the test runner binary as static arrays, IIRC.

So from the perspective of the CI, BPF_GCC is an additional test runner, built conditionally.

Now to the question wether we want to run this on all variants of the toolchain, I think yes. This way we are testing BPF emitted by GCC against kernels built by different toolchains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants