Skip to content

[BPF/SNAT] Improve code readability and complexity of the datapath #19712

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
Merged
merged 6 commits into from
Sep 20, 2022
Merged

[BPF/SNAT] Improve code readability and complexity of the datapath #19712

merged 6 commits into from
Sep 20, 2022

Conversation

sahid
Copy link
Contributor

@sahid sahid commented May 6, 2022

This is addressing issue #19660.

Split the snat_v{4,6}_process() functions into nat_snat_v{4,6}() and nat_rev_snat_v{4,6}().

The proposed change would not only improve the readability of the datapath code, but also should reduce the verifier complexity of the code path which is responsible for doing the rev-SNAT translations, as no new SNAT mapping needs to be find for this code path (only the lookup in the BPF SNAT map).

Fixes: #19660

Masquerading bpf mode -  Improve code readability and comlexity of the datapath.

@sahid sahid requested a review from a team as a code owner May 6, 2022 07:32
@sahid sahid requested a review from a team May 6, 2022 07:32
@sahid sahid requested review from a team as code owners May 6, 2022 07:32
@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 May 6, 2022
@sahid sahid requested review from kkourt and nbusseneau May 6, 2022 07:32
@sahid
Copy link
Contributor Author

sahid commented May 6, 2022

This is currently only addressing IPv4. I would appreciate comments before starting working on IPv6. Thanks a lot.

@sahid sahid mentioned this pull request May 6, 2022
Merged
@qmonnet

This comment was marked as outdated.

Sign in to view
@kkourt kkourt requested a review from brb May 6, 2022 10:37
@sahid sahid mentioned this pull request May 6, 2022
Closed
@sahid
Copy link
Contributor Author

sahid commented May 6, 2022

Looks like the complexity increased libbpf: Program too large (4102 insns), at most 4096 insns KI'm wondering if the problem is not coming from this commit: b3d8c9b

@sahid
Copy link
Contributor Author

sahid commented May 6, 2022 •
edited
Loading

Looks like the complexity increased libbpf: Program too large (4102 insns), at most 4096 insns KI'm wondering if the problem is not coming from this commit: b3d8c9b

I have made comparisons with an other PR executing k8s-1.22-kernel-4.19 [0] It seems my change is introducing new instructions with test_cilium_ipcache (not sure what is that sorry).

My change here which does factor the code of test_bpf_ct_prog should probably undefine IPCACHE4_PREFIXES and IPCACHE6_PREFIXES as they were not enabled originally.

[0] https://jenkins.cilium.io/job/Cilium-PR-K8s-1.22-kernel-4.19/1293/consoleText

@joestringer

This comment was marked as outdated.

Sign in to view
@sahid sahid force-pushed the issues/19660 branch 2 times, most recently from be815d5 to 0da1989 Compare May 9, 2022 14:09
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label May 9, 2022
@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 May 9, 2022
@joestringer

This comment was marked as outdated.

Sign in to view
@sahid
Copy link
Contributor Author

sahid commented May 10, 2022 •
edited
Loading

@brb Hello, the verifier does not look to have appreciated the split here:
3a137c0

I have isolated the commit

b6127504bd (HEAD -> issues/19660-wip) bpf/nat: split v4_process in v4_nat and v4_rev_nat
6b57b5257c (upstream/master) fqdn/dnsproxy: Improve error wrapping

Then ran verifier locally

vagrant@k8s1:~/go/src/github.com/cilium/cilium$ uname -r
4.19.190-0419190-generic

vagrant@k8s1:~/go/src/github.com/cilium/cilium$ make -C bpf && sudo test/bpf/verifier-test.sh
...
processed 1573 insns (limit 131072), stack depth 232
libbpf: load bpf program failed: Argument list too long
libbpf: Program too large (4524 insns), at most 4096 insns
libbpf: failed to load program 'tail_nodeport_nat_ipv4'

Any idea?

@sahid
Copy link
Contributor Author

sahid commented May 10, 2022

My feeling is that, we are using inline functions which duplicates the number of instructions... I start to think this refactoring will not work. We should probablly just consider to split the logic inside v4_process() based on dir.

This was referenced May 11, 2022
Closed
Closed
Closed
@sayboras

This comment was marked as outdated.

Sign in to view
@brb
Copy link
Member

brb commented May 11, 2022

Any idea?

Without seeing the full verifier log it's difficult to answer.

Anyway, my idea was to get rid of snat_v4_process() completely instead of making it call snat_v4_nat() / snat_v4_rev_nat() depending on the dir. The latter two would be called by functions which call snat_v4_process() today. This might require adding new tail calls.

@sayboras
Copy link
Member

sayboras commented Sep 12, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.23-kernel-5.4' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 30.001s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-5.4 so I can create one.

brb
brb reviewed Sep 13, 2022
View reviewed changes
@sahid sahid requested a review from brb September 13, 2022 14:33
@pchaigno pchaigno added area/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 Sep 15, 2022
@pchaigno

This comment was marked as outdated.

Sign in to view
@sahid sahid requested review from pchaigno and brb and removed request for brb and pchaigno September 16, 2022 07:35
@pchaigno
Copy link
Member

/test

brb
brb reviewed Sep 16, 2022
View reviewed changes
brb
brb reviewed Sep 16, 2022
View reviewed changes
@sahid
bpf/nat: introduce snat_v4_nat() and snat_v4_rev_nat functions
42ff671 
This split snat_v4_process() to introduce two functions: snat_v4_nat()
and snat_v4_rev_nat().  This does not change logic, it is expected to
reduce verifier complexity and improve code maintenability.

Fixes: cilium#19660
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid
bpf/nat: introduce snat_v6_nat() and snat_v6_rev_nat functions
91c4dc9 
This split snat_v6_process() to introduce two functions: snat_v6_nat()
and snat_v6_rev_nat().  This does not change logic, it is expected to
reduce verifier complexity and improve code maintenability.

Fixes: cilium#19660

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid
bpf/nat: introduce snat_v*_can_skip() nat and rev_nat
70d6dec 
In this desire of continuing improving code quality and verifier
complexity. This patch is splitting snat_v*_can_skip into two
functions.

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid
bpf/nat: introduce snat_v*_handle_mapping() nat and rev_nat
b968e9d 
In this desire of continuing improving code quality and verifier
complexity. This patch is splitting snat_v*_handle_mapping into two
functions.

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid
bpf/nat: introduce snat_v*_init_tuple() to avoid duplicate code
2a12b17 
This is introducting and using snat_v*_init_tuple() to set initial
values for the ct tuple.

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid
bpf/nat: remove unnecessary redondante nexthdr variable
90c672f 
It's redondante variable that does not look to bring any value.

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@industrialdiscipline.com>
@sahid sahid requested a review from brb September 16, 2022 13:06
@brb
Copy link
Member

brb commented Sep 19, 2022

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 20, 2022
@pchaigno pchaigno merged commit b4ed8cd into cilium:master Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno left review comments

@brb brb brb approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/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 ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

datapath: Split snat_v{4,6}_process() into two functions (SNAT and rev-SNAT)
7 participants
@sahid @qmonnet @joestringer @sayboras @brb @pchaigno @gentoo-root