bpf: convert majority of `bpf_elf_map` definitions to BTF map definitions #17640

ti-mo · 2021-10-19T12:29:45Z

Implements part of: #15149.

Best reviewed per commit.

Note that these were converted by hand using some macros, so would like an extra set of eyes to verify before/after.

This notably omits PROG_ARRAY and HASH_OF_MAPS maps because there is some extra work that needs to be done for those. They will be addressed in a follow-up.

bpf: convert majority of `bpf_elf_map` definitions to BTF map definitions

ti-mo · 2021-10-20T14:01:14Z

/test

ti-mo · 2021-10-22T13:56:58Z

/test

ti-mo · 2021-10-22T14:03:47Z

/test

ti-mo · 2021-10-25T12:17:35Z

/test

Rebased onto master for picking up e057d60

pchaigno

LGTM, only minor comments below.

I'm not super confident in my review of the Maglev inner map changes, but I see @borkmann is marked as a reviewer anyway.

bpf/bpf_sock.c

bpf/lib/lb.h

bpf/lib/maps.h

bpf/lib/nodeport.h

bpf/tests/prog_test/prog_test.go

bpf/include/bpf/loader.h

ti-mo · 2021-10-25T18:42:23Z

/test

ti-mo · 2021-10-26T09:48:01Z

/test

Job 'Cilium-PR-Runtime-net-next' hit: #17629 (98.39% similarity)

Job 'Cilium-PR-K8s-GKE' hit: #17270 (80.07% similarity)

Job 'Cilium-PR-K8s-1.16-net-next' hit: #17521 (98.50% similarity)

ti-mo · 2021-10-26T10:05:30Z

@borkmann

Checked libbpf outer map prepopulate behaviour; there is nothing to populate, so nothing is inserted.
Added __type(key, ..) in percpu array. Works, but no btf info is supplied to the kernel anyway if value is not a type. Providing a typedef instead of META_PIVOT should fix this, but not sure how to do this atm. Also not sure if worth it.
Removed dummy map name override, it's an inner map template and very short-lived.

qmonnet

Nice work! Two nits below (non-blocking).

As a side note, it's not often that we touch so many bpf/ files, and you could take this chance to update the copyright year in most of the files you changed.

bpf/lib/ids.h

pkg/maps/lbmap/maglev_inner_map.go

This is the first in a series of patches that converts most bpf_elf_map definitions to BTF map definitions. The changes in this patch are limited to maps that are supported by both cilium/ebpf and iproute2+libbpf. Further work is necessary to convert PROG_ARRAY and HASH_OF_MAPS map types given the legacy (iproute2) that needs to be handled. This patch contains mostly non-snowflakes; maps that were more involved to convert are addressed in separate commits. A __section_maps_btf macro was added to emit BTF map definitions into the well-known .maps section. Two other macros, __uint and __type were added, which are part of the BTF map definition spec/layout. The premise of BTF map definitions is that all struct fields must be pointers to save space in the .maps section, and (ab)using the BTF type graph to encode the actual values. In the case of __uint, the length of the array declared by the macro is set to the value given to the macro. The length of the array is then queried at load time and used as the 'value' of that struct member. A few more changes were required to functions that passed an explicit type of btf_elf_map *, which was changed to void *, as the BPF kernel-side map API also takes void *. Signed-off-by: Timo Beckers <timo@isovalent.com>

BTF map definitions allow for specifying a key and value type, which is used for pretty-printing map dumps. BPF arrays don't seem to support non-scalar keys when BTF info is attached, so some refactoring was needed to eliminate the struct key. Since get_encrypt_key() was only called with a zero argument, remove the argument and hardcode it in the function. Signed-off-by: Timo Beckers <timo@isovalent.com>

Broken out into a separate commit because bpf_ct_tests.c copies the map's address in a way that assumes the map is a bpf_elf_map. Access the map's global variable directly instead. patchELF() is also dropped since it is no longer necessary as of version 0.7.0 of cilium/ebpf. The trailing bytes are now carried in MapSpec.Extra and can be parsed or zeroed out by the caller. mark ct_key as static so it doesn't end up in .rodata. Signed-off-by: Timo Beckers <timo@isovalent.com>

This is an early example of what statically initialized PROG_ARRAY maps look like. This way of using .values is currently only supported by cilium/ebpf. While working on this, I noticed this program is never loaded into the kernel, which should probably be changed later as we can start making more use of PROG_RUN for testing purposes. Signed-off-by: Timo Beckers <timo@isovalent.com>

This patch moves over the v4 and v6 Maglev maps to BPF map definitions. NO_PREPOPULATE is no longer necessary given the 'inner_idx' field is not present in BTF map definitions, so it can no longer default to 0. This would normally lead to the inner map being inserted into the outer map at index 0, but BTF map definitions have the .values mechanism to populate outer maps instead. If .values is empty, no outer map writes will be performed. Both libbpf and cilium/ebpf immediately remove inner maps after the outer map has been created, so there is not much point in naming them. They can also not be pinned. The inner map name constants were removed from the agent, and bugtool no longer tries to open them from bpffs. The verifier-test image is upgraded simultaneously because the previous version was the pre-libbpf version of iproute2, which does not support relocating BTF map definitions into progbits yet. Unfortunately, this new version also contains a bug in how legacy bpf_elf_map inner maps are detected, see: https://lore.kernel.org/netdev/20211013143927.GA38205@Mem. In lieu of the iproute2 patch landing upstream, this bug is worked around by this very patch, by converting map-in-maps to BTF map definitions and no longer relying on bpf_elf_map compat code. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo · 2021-10-26T13:17:41Z

you could take this chance to update the copyright year in most of the files you changed.

@qmonnet Good point, done!

ti-mo · 2021-10-26T15:24:04Z

Marked as ready, all k8s tests were green before and no functional changes were made during the last round of feedback. Kicking off a last round of tests just to make sure. (it takes hours...)

ti-mo · 2021-10-26T15:24:12Z

/test

Job 'Cilium-PR-K8s-1.19-kernel-5.4' hit: #17069 (92.67% similarity)

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

Click to show.

Test Name

K8sFQDNTest Restart Cilium validate that FQDN is still working

Failure Output

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

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 Oct 19, 2021

ti-mo added the release-note/misc This PR makes changes that have no direct user impact. label Oct 19, 2021

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 Oct 19, 2021

ti-mo force-pushed the pr/tb/btf-map-definitions branch 5 times, most recently from ad79401 to 1654233 Compare October 20, 2021 14:01

ti-mo force-pushed the pr/tb/btf-map-definitions branch 2 times, most recently from 12af5df to 769163a Compare October 22, 2021 14:02

ti-mo marked this pull request as ready for review October 22, 2021 14:03

ti-mo requested a review from a team October 22, 2021 14:03

ti-mo requested review from a team as code owners October 22, 2021 14:03

ti-mo requested review from borkmann and nebril October 22, 2021 14:04

maintainer-s-little-helper bot assigned borkmann Oct 22, 2021

ti-mo requested a review from ldelossa October 22, 2021 14:04

maintainer-s-little-helper bot assigned nebril and ldelossa and unassigned nebril Oct 22, 2021

ti-mo force-pushed the pr/tb/btf-map-definitions branch from 769163a to a071146 Compare October 25, 2021 12:17

pchaigno approved these changes Oct 25, 2021

View reviewed changes

pchaigno added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. sig/loader Impacts the loading of BPF programs into the kernel. labels Oct 25, 2021

ti-mo force-pushed the pr/tb/btf-map-definitions branch from a071146 to afc9a72 Compare October 25, 2021 15:54

ti-mo force-pushed the pr/tb/btf-map-definitions branch from afc9a72 to 44238da Compare October 26, 2021 09:47

This was referenced Oct 26, 2021

CI: RuntimeLB With Containers validates service recovery on restart #17629

Closed

CI: K8sIstioTest Istio Bookinfo Demo Tests bookinfo inter-service connectivity #17270

Closed

qmonnet approved these changes Oct 26, 2021

View reviewed changes

bpf/lib/ids.h Show resolved Hide resolved

pkg/maps/lbmap/maglev_inner_map.go Outdated Show resolved Hide resolved

maintainer-s-little-helper bot mentioned this pull request Oct 26, 2021

CI: K8sPolicyTest Basic Test Validate to-entities policies Validate toEntities All #17521

Closed

ti-mo added 5 commits October 26, 2021 15:11

ti-mo force-pushed the pr/tb/btf-map-definitions branch from 44238da to 8bf4206 Compare October 26, 2021 13:17

ti-mo added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 26, 2021

maintainer-s-little-helper bot mentioned this pull request Oct 26, 2021

v1.11: CI: K8sDatapathConfig Encapsulation Check connectivity with sockops and VXLAN encapsulation #17069

Closed

borkmann approved these changes Oct 27, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned borkmann Oct 27, 2021

kkourt merged commit 789eef0 into cilium:master Oct 27, 2021

ti-mo deleted the pr/tb/btf-map-definitions branch October 27, 2021 08:55

pchaigno mentioned this pull request Nov 5, 2021

k8s-upstream: SCTP tests failing #17796

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpf: convert majority of `bpf_elf_map` definitions to BTF map definitions #17640

bpf: convert majority of `bpf_elf_map` definitions to BTF map definitions #17640

ti-mo commented Oct 19, 2021

ti-mo commented Oct 20, 2021

ti-mo commented Oct 22, 2021

ti-mo commented Oct 22, 2021

ti-mo commented Oct 25, 2021 •

edited

pchaigno left a comment

ti-mo commented Oct 25, 2021

ti-mo commented Oct 26, 2021 •

edited by maintainer-s-little-helper bot

ti-mo commented Oct 26, 2021 •

edited

qmonnet left a comment

ti-mo commented Oct 26, 2021

ti-mo commented Oct 26, 2021

ti-mo commented Oct 26, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

bpf: convert majority of bpf_elf_map definitions to BTF map definitions #17640

bpf: convert majority of bpf_elf_map definitions to BTF map definitions #17640

Conversation

ti-mo commented Oct 19, 2021

ti-mo commented Oct 20, 2021

ti-mo commented Oct 22, 2021

ti-mo commented Oct 22, 2021

ti-mo commented Oct 25, 2021 • edited

pchaigno left a comment

Choose a reason for hiding this comment

ti-mo commented Oct 25, 2021

ti-mo commented Oct 26, 2021 • edited by maintainer-s-little-helper bot

ti-mo commented Oct 26, 2021 • edited

qmonnet left a comment

Choose a reason for hiding this comment

ti-mo commented Oct 26, 2021

ti-mo commented Oct 26, 2021

ti-mo commented Oct 26, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

bpf: convert majority of `bpf_elf_map` definitions to BTF map definitions #17640

bpf: convert majority of `bpf_elf_map` definitions to BTF map definitions #17640

ti-mo commented Oct 25, 2021 •

edited

ti-mo commented Oct 26, 2021 •

edited by maintainer-s-little-helper bot

ti-mo commented Oct 26, 2021 •

edited

ti-mo commented Oct 26, 2021 •

edited by maintainer-s-little-helper bot