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

Extend K8sVerifier to maximize program sizes on 4.19 and net-next kernels #14451

Merged
merged 6 commits into from Jan 25, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Dec 18, 2020

The goal of this pull request is to extend K8sVerifier's coverage specifically to test datapath configurations specific to newer 4.19 and net-next kernels (K8sVerifier today only tests 4.9 options). The following summarizes the six commits:

  1. Ensure we can compile-test tunneling by not hardcoding ENCAP_IFINDEX. Required to be able to load bpf_xdp without tunneling.
  2. Refactor our MAX_*_OPTIONS macros and enable more options on 4.9.
  3. Adapt MAX_*_OPTIONS to the given kernel to always try to maximize the program sizes.
  4. Fix macros used in is_defined which requires a value of 1 to be detected as defined.
  5. Mimic cilium-agent's behavior when setting mcpu.
  6. Changes datapath options to work around Complexity issue in bpf_lxc with kubeproxy-free + partial hostReachableServices (tcp or udp only) #14234.

Please see commits for details.

@pchaigno pchaigno added sig/loader Impacts the loading of BPF programs into the kernel. area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. dont-merge/blocked Another PR must be merged before this one. needs-backport/1.8 labels Dec 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Dec 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.2 Dec 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.7 Dec 18, 2020
@pchaigno pchaigno changed the title Extend K8sVerifier to maximize program size on 4.19 and net-next kernels Extend K8sVerifier to maximize program sizes on 4.19 and net-next kernels Dec 18, 2020
@pchaigno pchaigno force-pushed the pr/pchaigno/bpf-max-options-newer-kernels branch from 6d7cf1e to d387d77 Compare December 18, 2020 15:06
Base automatically changed from pr/pchaigno/fix-prefilter-ipv6-regression to master December 21, 2020 12:42
@pchaigno pchaigno force-pushed the pr/pchaigno/bpf-max-options-newer-kernels branch from d387d77 to 00e46e3 Compare January 4, 2021 09:38
@pchaigno pchaigno removed the dont-merge/blocked Another PR must be merged before this one. label Jan 4, 2021
@pchaigno pchaigno force-pushed the pr/pchaigno/bpf-max-options-newer-kernels branch 2 times, most recently from 6fedf16 to 3767097 Compare January 4, 2021 15:13
@pchaigno pchaigno marked this pull request as ready for review January 4, 2021 17:46
@pchaigno pchaigno requested a review from a team January 4, 2021 17:46
@pchaigno pchaigno requested review from a team as code owners January 4, 2021 17:46
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. (Just some minor non-blocking comments.)

bpf/Makefile Show resolved Hide resolved
bpf/node_config.h Show resolved Hide resolved
bpf/Makefile Outdated Show resolved Hide resolved
bpf/Makefile Outdated Show resolved Hide resolved
@pchaigno pchaigno force-pushed the pr/pchaigno/bpf-max-options-newer-kernels branch from 3767097 to 38d46ee Compare January 18, 2021 16:16
@aanm aanm added this to Needs backport from master in 1.9.3 Jan 20, 2021
The NAT46 change in node_config.h is required to define CONNTRACK is
ENABLE_NAT46 is defined. This wasn't previously required because
ENABLE_NAT46 was only defined for bpf_xxx files that have CONNTRACK
defined in their own header file (e.g., ep_config.h).

Signed-off-by: Paul Chaignon <paul@cilium.io>
The make target for the BPF programs takes a KERNEL environment variable.
This variable is used to try and maximize the program size for each
kernel, by enabling as many features as possible. This new environment
variable is also used in the existing K8sVerifier test.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/bpf-max-options-newer-kernels branch from 38d46ee to 6c17e32 Compare January 22, 2021 22:53
In the datapath, we have two ways to check if a macro is defined:
the usual defined()/#ifdef and is_defined(). The later is often used in
C predicates or in tail call conditions. It expects the macro to be
defined with a value, typically #define ENABLE_FEATURE_X 1. Without this
non-zero value, is_defined(ENABLE_FEATURE_X) returns false.

The list of macros that are used in is_defined can be extracted with
the following command:

    git grep -hoP "(?<=is_defined\()[_A-Z0-9]+" bpf/ | sort | uniq

Nevertheless, in this commit, to prevent mistakes in future addition,
I've added a value to all macros. This is automatically achieved by
replacing ':' by '=1 ' instead of just a space. It however requires that
all macros be followed by ':'.

Signed-off-by: Paul Chaignon <paul@cilium.io>
When compiling our datapath (base and endpoint's), cilium-agent selects
mcpu=v1 or v2 depending on the kernel version. Up until now, the object
files generated for test-verifier.sh had mcpu=probe. This commit changes
it to match what cilium-agent does.

Signed-off-by: Paul Chaignon <paul@cilium.io>
If ENABLE_HOST_SERVICES_FULL is undefined, additional code is compiled
in our datapath, leading us to run into known complexity issue #14234.
We only need to work around this issue on the latest kernels.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/bpf-max-options-newer-kernels branch from 6c17e32 to 1a59e53 Compare January 22, 2021 22:59
@pchaigno
Copy link
Member Author

test-me-please

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 23, 2021
@aanm aanm merged commit 5cf67ef into master Jan 25, 2021
@aanm aanm deleted the pr/pchaigno/bpf-max-options-newer-kernels branch January 25, 2021 09:38
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jan 26, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport done to v1.8 in 1.8.7 Jan 28, 2021
@christarazi christarazi moved this from Needs backport from master to Backport done to v1.9 in 1.9.4 Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
No open projects
1.8.7
Backport done to v1.8
1.9.4
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

6 participants