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.

Sign up for GitHub

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

Jump to bottom

bpf: lb: introduce an optimized CT lookup #22936

Merged
merged 9 commits into from Feb 17, 2023
Merged

bpf: lb: introduce an optimized CT lookup #22936

merged 9 commits into from Feb 17, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Jan 5, 2023 •
edited

This splits off the CT improvements from #22978 for easier reviewing (ie no need to follow all the DSR changes in that PR). They are needed to slim down the instruction count in the nodeport path.

The main idea is to extract a fully populated CT tuple in the service lookup path once, and thus avoid multiple L4 port extractions in the ct_lookup() calls. As we need to extract the dest port for the Service lookup anyway, we can also extract the source port at this time without extra cost.

I expect that we can use the new ct_lb_lookup() helpers in more locations over time, and presumably also rename it at that point.

@maintainer-s-little-helper 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 Jan 5, 2023
@julianwiedmann julianwiedmann force-pushed the bpf-ct-improvements branch 2 times, most recently from 584c46c to 43efdc5 Compare January 5, 2023 10:29
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann reopened this Jan 5, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jan 5, 2023
@maintainer-s-little-helper 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 Jan 5, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/complexity-issue Relates to BPF complexity or program size issues labels Feb 13, 2023
@julianwiedmann julianwiedmann changed the title WIP BPF CT improvements bpf: lb: introduce an optimized CT lookup Feb 13, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review February 13, 2023 18:45
@julianwiedmann julianwiedmann requested a review from a team as a code owner February 13, 2023 18:45
@ldelossa ldelossa mentioned this pull request Feb 14, 2023
Merged
@julianwiedmann
Copy link
Member Author

/test-1.16-4.19

pchaigno
pchaigno approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM. Last comment is mixing a small logic change with a fair amount of refactoring; probably would have been easier to review if the two were separate. That said, the rest of the commit history is 👌

@pchaigno
Copy link
Member

I see this is labelled release-note/minor but we don't expect any user impact, do we? Should it be release-note/misc?

@julianwiedmann julianwiedmann added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Feb 16, 2023
@julianwiedmann
Copy link
Member Author

I see this is labelled release-note/minor but we don't expect any user impact, do we? Should it be release-note/misc?

yep, must have fat-fingered. Thanks!

ldelossa
ldelossa approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM, couple questions.

bpf/bpf_lxc.c Show resolved Hide resolved
bpf/lib/lb.h Show resolved Hide resolved
@julianwiedmann
bpf: lb: use lb6_extract_tuple() in SVC lookup paths
28afd20 
As part of the nodeport SVC lookup, lb6_extract_key() currently extracts
the packet's destination port. Refactor this code path to use
lb6_extract_tuple() instead, thus loading _both_ L4 ports. We can use this
in a subsequent patch to slim down the CT lookup code.

lb6_extract_key() turns into a much simpler helper that only fills the
lb6_key.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: fine-tune extract_l4_port()
8d70d50 
This helper is now only used for IPv4 packets. Remove the IPv6-specific
parts.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: pull ipv4_handle_fragmentation() into lb4_extract_tuple()
638d6f9 
We already have a validated IPv4 header, so avoid jumping through
ipv4_ct_extract_l4_ports() for loading the ports.

Stick close to how lb4_extract_l4_port() lays out the code, as we want to
start using lb4_extract_tuple() in more places and the 4.19 verifier is
very picky about it.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: use lb4_extract_tuple() in SVC lookup paths
752d8a2 
Same as for lb6_extract_tuple().

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: de-dup and simplify l4_xlate
64f40ee 
Trust that earlier stages have already checked the L4 protocol, and we're
not suddenly processing a request with non-SVC protocol.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: have lb_local() update CT tuple with backend port
9030464 
The SVC-to-backend DNAT code already updates the .daddr in the CT tuple.
Also update the backend port, so that the nodeport code can use this tuple
as-is to create an CT entry for RevDNAT of non-DSR connections.

Slightly pull down the .daddr update in lb6_local(), to keep the code in
sync with lb4_local() and have the whole tuple update in one place.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: conntrack: use enum ct_dir for CT_* direction
dd525c7 
Enforce proper types.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: conntrack: introduce an optimized CT lookup for LB
ae1eaa6 
Allow the LB paths to perform a CT lookup without validating the
L3 protocol or extracting the L4 ports. This helps to reduce instruction
count in the nodeport code.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: use new ct_lb_lookup() helpers
287f1c3 
As the LB paths are doing their own L3 protocol checks and also extract
the L4 ports on their own, we can switch lb_local() and all the nodeport
code to use the optimized CT lookup helpers.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

LGTM. Last comment is mixing a small logic change with a fair amount of refactoring; probably would have been easier to review if the two were separate.

Yeah, that's fair - probably a bit too big to refactor + introduce the new usage in the same commit. Split it up now.

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 17, 2023
@pchaigno pchaigno merged commit 284f62f into cilium:master Feb 17, 2023
@julianwiedmann julianwiedmann deleted the bpf-ct-improvements branch February 17, 2023 15:38
@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 Feb 17, 2023
@julianwiedmann julianwiedmann mentioned this pull request Apr 17, 2023
Merged
6 tasks
@julianwiedmann julianwiedmann added the backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. label Apr 24, 2023
@thorn3r thorn3r mentioned this pull request May 17, 2023
Merged
@borkmann borkmann mentioned this pull request Jun 23, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gentoo-root gentoo-root gentoo-root left review comments

@ldelossa ldelossa ldelossa approved these changes

@pchaigno pchaigno pchaigno approved these changes

@NikAleksandrov NikAleksandrov Awaiting requested review from NikAleksandrov NikAleksandrov was automatically assigned from cilium/sig-datapath

@brb brb Awaiting requested review from brb

Assignees
No one assigned
Labels
backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/complexity-issue Relates to BPF complexity or program size issues release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@julianwiedmann @pchaigno @gentoo-root @ldelossa