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

bpf: Additional tail calls for IPvX-only setups #17573

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Oct 11, 2021

When both IPv4 and IPv6 are enabled, we split the {to/from}-container BPF programs into two code paths, one for each IP family, to reduce program size and complexity. Because our existing K8sVerifier test only covers the IPv4+IPv6 configuration, new complexity and program size issues sneaked in for the IPvX-only setups.

These new issues occur when {to/from}-container contain both the initial IP parsing code and the IPv4 (resp. IPv6---we have one issue per family) code path. Splitting these programs such that they only contain the initial IP parsing code is enough to fix these issues.

These complexity and program issues were found by ongoing work at #17470. That same PR was also used to validate the fix.

Fixes: #17486.
Fixes: #17490.
Fixes: #17491.

Fix several complexity and program size issues when only one of IPv4/IPv6 is enabled.

@pchaigno pchaigno added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. kind/complexity-issue Relates to BPF complexity or program size issues labels Oct 11, 2021
@pchaigno pchaigno requested review from a team and jrfastab October 11, 2021 19:26
@kkourt kkourt self-requested a review October 12, 2021 12:16
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.

LGTM.

I think OR-ing ENABLE_IPV4 and ENABLE_IPV6 is a good idea even if it will always be true to maintain the structure of the original code in case we want to revisit this.

For some additional context, we have done something similar in 1.8 only: #16965. Should we be proactive and backport this to 1.9 and 1.10?

@pchaigno
Copy link
Member Author

For some additional context, we have done something similar in 1.8 only: #16965. Should we be proactive and backport this to 1.9 and 1.10?

I agree. I'll backport the test first. If it fails in v1.9/1.10, then we know we need to backport and mark as a bugfix in release notes. If it doesn't fail, then I'll backport anyway but it's not a bugfix in release notes. I've set a reminder to not forget.

When both IPv4 and IPv6 are enabled, we split the to/from-container BPF
programs into two code paths, one for each IP family, to reduce program
size and complexity. Because our existing K8sVerifier test only covers
the IPv4+IPv6 configuration, new complexity and program size issues
sneaked in for the IPvX-only setups.

These new issues occur when to/from-container contain both the initial IP
parsing code and the IPv4 (resp. IPv6---we have one issue per family)
code path. Splitting these programs such that they only contain the
initial IP parsing code is enough to fix these issues.

Signed-off-by: Paul Chaignon <paul@cilium.io>
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Changes look good

@maintainer-s-little-helper
Copy link

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 10.128.15.197:80 from testclient-f7dxz

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 13, 2021
@joestringer joestringer merged commit 1b6a98c into cilium:master Oct 13, 2021
@pchaigno pchaigno deleted the fix-complexity-issues-ipv46 branch October 13, 2021 19:45
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic. kind/complexity-issue Relates to BPF complexity or program size issues release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
5 participants