bpf: dynamic-width static data inlining #25195

ti-mo · 2023-04-28T13:04:30Z

Historically, the largest data type used for declaring global data was uint32.
Cilium's iproute2 fork had patches that made the assumption all values are
32 bits, so this behaviour was replicated when it was ported to Go. This only
works well if the largest variable emitted to .data is 32 bits wide. Any
smaller and the section's alignment would drop, causing the inliner to read
part of the next variable. Any larger and only part of a u64 would be read.

Since we now no longer rely on iproute2 for loading any ELFs, we can start
changing the way this works. In Go, we have access to .data's BTF Datasec,
so we can be a bit smarter about it and detect how wide each variable is,
so we can accurately copy out the bits needed.

This patch introduces a uint64 global data macro, DEFINE_U64, and updates
DEFINE_IPV6 to use it. This cuts down on the amount of instructions needed
to carry ipv6 values in the instruction stream from 8 to 4.

Closes: #7123

Reduce amount of bpf instructions needed for handling ipv6 addresses

pkg/datapath/linux/config/config_test.go

gentoo-root · 2023-05-01T11:39:50Z

pkg/datapath/loader/template.go

-		result["LXC_IP_3"] = sliceToBe32(ipv6[8:12])
-		result["LXC_IP_4"] = sliceToBe32(ipv6[12:16])
+		result["LXC_IP_1"] = sliceToBe32(ipv6[0:8])
+		result["LXC_IP_2"] = sliceToBe32(ipv6[8:16])


Why Be32 if it's 64-bit now? This won't work, will it?

Good catch, was a little rushed writing this. This made me realize the ELF substitution code also needed 64-bit support added.

gentoo-root · 2023-05-01T11:42:44Z

pkg/datapath/loader/template.go

-		result["LXC_IP_2"] = sliceToBe32(ipv6[4:8])
-		result["LXC_IP_3"] = sliceToBe32(ipv6[8:12])
-		result["LXC_IP_4"] = sliceToBe32(ipv6[12:16])
+		result["LXC_IP_1"] = sliceToBe32(ipv6[0:8])


General question: why isn't the same mechanism used for NAT_46X64_PREFIX? Can we still optimize NAT_46X64_PREFIX as well to use two 64-bits instead of four 32-bits?

Yep, the way it's implemented looks a bit strange to me. Instead of assigning individual bytes, why don't we write a single value to v6addr->p1, which is a u32? @borkmann

I figure it might be related to endianness and making sure the same byte goes into the same slot in the address. The way sliceToBe32 and friends functions is quite subtle, and the ELF rewriter also potentially byte-swaps when converting u16/32/64 to []byte, which bit me a few times while working on this PR. Some of it will go away when we remove ELF rewriting, though, I'll revisit it then.

ti-mo · 2023-05-02T13:47:46Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing and endpoint routes

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-host-8d6b6

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/1947/

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

Then please upload the Jenkins artifacts to that issue.

Missing bpf_cpu_to_be64 for follow-up commit. Signed-off-by: Timo Beckers <timo@isovalent.com>

This patch adds support for 64-bit wide ELF substitutions, to be used in a follow-up patch. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo · 2023-05-03T07:50:32Z

/test

pchaigno

LGTM. Thanks for the nice commit history!

pkg/bpf/collection.go

pkg/bpf/collection_test.go

Historically, the largest data type used for declaring global data was uint32. Cilium's iproute2 fork had patches that made the assumption all values are 32 bits, so this behaviour was replicated when it was ported to Go. This only works well if the largest variable emitted to .data is 32 bits wide. Any smaller and the section's alignment would drop, causing the inliner to read part of the next variable. Any larger and only part of a u64 would be read. Since we now no longer rely on iproute2 for loading any ELFs, we can start changing the way this works. In Go, we have access to .data's BTF Datasec, so we can be a bit smarter about it and detect how wide each variable is, so we can accurately copy out the bits needed. This patch introduces a uint64 global data macro, DEFINE_U64, and updates DEFINE_IPV6 to use it. This cuts down on the amount of instructions needed to carry ipv6 values in the instruction stream from 8 to 4. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo · 2023-05-04T13:03:07Z

checkpatch failure can be ignored, it returns non-zero even if there are only suggestions left. Not addressing those since they're about copied kernel headers, and false-positive.

Pushed a small doc change and touched a self-contained unit test. This is ready to 🚢.

joestringer · 2023-05-04T15:20:42Z

/test

joestringer

API codeowners changes are trivial, approving on their behalf.

CI is green, ready to merge.

nikhiljha · 2023-09-05T09:00:43Z

Hello! I think this broke IPv6 (at least with our configuration) in Cilium 1.14 and up, which we confirmed with a bisect (and also this touches some macros / functions used by that logic, see #27898). We tried to revert this but it's in a very load bearing file so that did not go very well. Does anyone more familiar with what this PR have any insight about what might be wrong?

ti-mo added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. labels Apr 28, 2023

ti-mo requested a review from a team as a code owner April 28, 2023 13:04

ti-mo requested review from gentoo-root and lmb April 28, 2023 13:04

ti-mo mentioned this pull request Apr 28, 2023

Improve static data load support #7123

Closed

borkmann requested a review from joestringer April 28, 2023 13:40

ti-mo force-pushed the tb/dynamic-globals branch from e1bb7f2 to 0e37422 Compare April 28, 2023 13:42

ti-mo requested a review from a team as a code owner April 28, 2023 13:42

gentoo-root requested changes May 1, 2023

View reviewed changes

ti-mo force-pushed the tb/dynamic-globals branch from 0e37422 to ad34306 Compare May 2, 2023 13:40

ti-mo requested a review from a team as a code owner May 2, 2023 13:40

ti-mo requested a review from nathanjsweet May 2, 2023 13:40

ti-mo force-pushed the tb/dynamic-globals branch from ad34306 to 0cc5166 Compare May 2, 2023 13:41

nathanjsweet removed their request for review May 2, 2023 19:17

ti-mo added 2 commits May 3, 2023 09:47

bpf/lib: sync endian.h with upstream

22fb40b

Missing bpf_cpu_to_be64 for follow-up commit. Signed-off-by: Timo Beckers <timo@isovalent.com>

datapath,elf: support 64-bit ELF substitutions

2765086

This patch adds support for 64-bit wide ELF substitutions, to be used in a follow-up patch. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo force-pushed the tb/dynamic-globals branch from 0cc5166 to a6f71fe Compare May 3, 2023 07:49

ti-mo requested a review from dylandreimerink May 3, 2023 13:48

pchaigno approved these changes May 3, 2023

View reviewed changes

maintainer-s-little-helper bot requested a review from gentoo-root May 3, 2023 18:49

lmb reviewed May 4, 2023

View reviewed changes

pkg/bpf/collection.go Show resolved Hide resolved

pkg/bpf/collection_test.go Show resolved Hide resolved

dylandreimerink approved these changes May 4, 2023

View reviewed changes

gentoo-root approved these changes May 4, 2023

View reviewed changes

ti-mo force-pushed the tb/dynamic-globals branch from a6f71fe to e655805 Compare May 4, 2023 12:58

ti-mo added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 4, 2023

joestringer added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels May 4, 2023

joestringer approved these changes May 5, 2023

View reviewed changes

joestringer merged commit d10e8eb into cilium:main May 5, 2023
55 of 57 checks passed

joestringer added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 5, 2023

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 May 5, 2023

ti-mo deleted the tb/dynamic-globals branch May 5, 2023 08:38

julianwiedmann mentioned this pull request May 8, 2023

Fix implicit conversion warning in DSR with GENEVE #25299

Merged

8 tasks

nikhiljha mentioned this pull request Sep 5, 2023

ipv6 endpoint connectivity regression 1.13.6 -> 1.15.0-pre.0 #27898

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpf: dynamic-width static data inlining #25195

bpf: dynamic-width static data inlining #25195

ti-mo commented Apr 28, 2023

gentoo-root May 1, 2023

ti-mo May 2, 2023

gentoo-root May 1, 2023

ti-mo May 2, 2023

ti-mo May 2, 2023 •

edited

ti-mo commented May 2, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

ti-mo commented May 3, 2023

pchaigno left a comment

ti-mo commented May 4, 2023

joestringer commented May 4, 2023

joestringer left a comment •

edited

nikhiljha commented Sep 5, 2023

bpf: dynamic-width static data inlining #25195

bpf: dynamic-width static data inlining #25195

Conversation

ti-mo commented Apr 28, 2023

gentoo-root May 1, 2023

Choose a reason for hiding this comment

ti-mo May 2, 2023

Choose a reason for hiding this comment

gentoo-root May 1, 2023

Choose a reason for hiding this comment

ti-mo May 2, 2023

Choose a reason for hiding this comment

ti-mo May 2, 2023 • edited

Choose a reason for hiding this comment

ti-mo commented May 2, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

ti-mo commented May 3, 2023

pchaigno left a comment

Choose a reason for hiding this comment

ti-mo commented May 4, 2023

joestringer commented May 4, 2023

joestringer left a comment • edited

Choose a reason for hiding this comment

nikhiljha commented Sep 5, 2023

ti-mo May 2, 2023 •

edited

ti-mo commented May 2, 2023 •

edited by maintainer-s-little-helper bot

joestringer left a comment •

edited