Move map management to `BpfBytecode` #2988

viktormalik · 2024-02-09T08:13:15Z

The current map management using MapManager/IMap/Map/FakeMap is quite difficult to read in the code as map settings are scattered across multiple files. This PR attempts to unify everything in BpfBytecode, where it logically belongs.

Maps are stored by their name (map ids are not used anymore) and represented using a new BpfMap class. The class contains struct bpf_map which is libbpf map representation containing basic map information (map type, max entries, key and value size) which libbpf is able to parse from BTF stored in the generated ELF. In addition, BpfMap stores map fd. Once we let libbpf auto-create maps for us in future, we will be able to drop manual map creation and fd will also be stored in struct bpf_map. Logically, the new BpfMap class replaces the previous Imap/Map classes.

The PR migrates all map usages to the new map management. The migration had to deal with two kinds of maps:

Internal maps which usually only use fd and therefore the migration was mostly straightforward.
Async map actions (print, clear, and zero) which came with two problems: (1) async actions need map ids which were dropped and (2) some information needed (e.g. key/value type) is not stored in BpfMap. Since I wanted to keep the generated BPF programs decoupled from runtime as much as possible, I decided to store the above information in RequiredResources which are accessible by async actions. Part of the information was already there and the PR adds map name<->id mappings (for maps used inside async actions).

Finally, MapManager, IMap, Map, and FakeMap are dropped.

See individual commit messages for more details.

Note: this is the last part of #2911. It's quite big but unfortunately, I wasn't able to split it into smaller parts as it wouldn't make much sense to me. If you still prefer to post it per-commit, let me know.

Checklist

Language changes are updated in man/adoc/bpftrace.adoc and if needed in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

src/bpftrace.cpp

viktormalik · 2024-02-13T07:02:39Z

For some reason, there are random failures of the call.ustack_stack_mode_env* runtime tests. Restarting the job usually helps but I don't remember seeing such failures before so they may be caused by something in this PR. I'll try to have a look.

viktormalik · 2024-02-14T11:07:38Z

For some reason, there are random failures of the call.ustack_stack_mode_env* runtime tests. Restarting the job usually helps but I don't remember seeing such failures before so they may be caused by something in this PR. I'll try to have a look.

Unfortunately, I'm not able to reproduce the issue locally (even when using Nix).

ajor · 2024-02-14T15:31:15Z

What kinds of failures were they?

ajor

Overall looks good to me.

There do seem to be a lot of map lookups going on throughout the whole process, which might be slowing us down more than necessary.

e.g. in the output path (where performance counts), we have a multi-step process to obtain a BpfMap object:

Ringbuffer -> print->mapid
resources.maps_by_id[mapid] -> map_name
bytecode.getMap(map_name) -> BpfMap

Also resources.map_key and resources.map_type being two separate map lookups.

src/bpfprogram.cpp

src/bpfmap.cpp

src/required_resources.cpp

src/bpftrace.cpp

viktormalik · 2024-02-15T06:56:03Z

What kinds of failures were they?

IIRC there was no stack printed (just empty output).

viktormalik · 2024-02-15T11:34:53Z

Overall looks good to me.

There do seem to be a lot of map lookups going on throughout the whole process, which might be slowing us down more than necessary.

That's a good point, although I'm wondering if we can really avoid that.

e.g. in the output path (where performance counts), we have a multi-step process to obtain a BpfMap object:

Ringbuffer -> print->mapid

This is necessary b/c mapid is stored in codegen. I don't see a way to avoid this.

resources.maps_by_id[mapid] -> map_name

bytecode.getMap(map_name) -> BpfMap

Probably the most efficient would be to have mapid -> BpfMap mapping but the problem is that we need mapid before codegen but only create BpfMap after codegen. We could create an extra mapid -> BpfMap mapping (perhaps in BpfBytecode) after BpfMap objects are created. I can give it a try. If you have other idea how to resolve this, let me know.

Also resources.map_key and resources.map_type being two separate map lookups.

That's a good point. We could probably aggregate them in some structure like MapInfo in RequiredResources (together with other info such as map ids, (l)hist args, etc.).

viktormalik · 2024-02-15T15:24:50Z

Thanks for the review @ajor! I implemented all of the comments, noticeable changes in the new version are:

moved linking of required_resources.cpp out of libruntime to libaot only,
aggregated map information in RequiredResources to a single struct,
created a new map BpfBytecode::maps_by_id_ to allow efficient lookup of BpfMap objects by map ids.

src/bpfbytecode.cpp

ajor

Looks great!

Just spotted few extra copies we can eliminate. I'm extra suspicious whenever I see auto x = because it's easy to hide these expensive copies (not saying that I don't code like this myself!)

src/bpfprogram.cpp

src/bpfbytecode.cpp

src/bpftrace.cpp

src/output.cpp

viktormalik · 2024-02-15T19:35:57Z

Just spotted few extra copies we can eliminate. I'm extra suspicious whenever I see auto x = because it's easy to hide these expensive copies (not saying that I don't code like this myself!)

Thanks, those are great catches! I try to avoid auto x = , too, but sometimes my brain falls into a just-auto-everything mode and that's when these happen 😆

Anyways, I'd like to wait for @danobi's review before merging this.

danobi

only looked at first 3 commits. might get more time this weekend to finish

src/bpfbytecode.h

src/bpfmap.h

src/bpfbytecode.cpp

danobi

few comments, but great cleanup!

src/bpfbytecode.h

src/ast/passes/codegen_llvm.cpp

src/required_resources.cpp

src/required_resources.h

src/bpfbytecode.h

src/CMakeLists.txt

danobi · 2024-02-16T18:36:49Z

src/aot/CMakeLists.txt

@@ -1,4 +1,4 @@
-add_library(aot aot.cpp)
+add_library(aot aot.cpp ${CMAKE_SOURCE_DIR}/src/required_resources.cpp)


Yeah this is a bit unfortunate. I feel like cmakelists.txt should try not to reach up to parent or sibling dirs

I don't like it either. But I couldn't come with any other solution, besides adding the dummy constructor (see the other thread).

Should required_resources.cpp live in the aot directory, since that's where it's used?

Possibly but we wouldn't have required_resources.cpp next to required_resources.h.

I was thinking of moving the header too, but I suppose it's used in more than just AOT so that doesn't make sense.

How about creating a new CMake library target just for required_resources? Then aot can link against that - would it solve the unused symbol issues?

Agreed - separate target is probably cleanest solution

Good idea, done.

bpf_object is libbpf's structure for representing a collection of BPF programs, maps, etc. Typically, it's parsed from a single ELF file. Since we now emit BTF information on maps into the produced ELF, we can let libbpf parse it for us so that don't have to store it elsewhere (currently in IMap/Map/FakeMap/MapManager). This is another step towards libbpf-based loading and attachment of programs. Eventually, we'd like to get into the state when BpfBytecode only contains bpf_object and everything is managed by libbpf.

The basic map information (map type, max entries, key and value size) is now stored inside the generated ELF in the BTF form. Thanks to that, libbpf is able to parse that information and store it inside bpf_object. We can retrieve the information and use it to create BPF maps from BpfBytecode (instead of from RequiredResources). This has several advantages: 1. We only need to store minimal map information (name and fd). This is done in BpfBytecode (where it logically belongs) using a new BpfMap class, instead of MapManager. We will eventually be able to drop MapManager. 2. Once we let libbpf auto-create maps for us, we will need minimal code adjustments (we'll just remove the manual map creation). The new BpfMap class logically replaces the previous Imap/Map classes. Since map information is used in many places, we do not remove MapManager and Map/Imap at once (the commit would be just too large). To allow removing it incrementally, we (1) copy the map fds from BpfBytecode maps to MapManager maps and (2) store names for builtin maps in MapManager. Once all usages of MapManager maps are gone, we will drop this code. The only map manipulation code that is migrated at once is filling of the printf data map. This is because it is done in the same method as the data map is created (RequiredResources::prepareFormatStringDataMap) and therefore it cannot be kept while moving the map creation code.

All manipulations of internal maps now use maps from BpfBytecode instead of MapManager. This also allows to move the MapManager::Type enum for internal maps to a new MapType enum located in bpfmap.h. This also removes some code from RequiredResources and MapManager which was using (now missing) MapManager::Type. Codegen tests for internal maps (stacks) now use BpfBytecode maps rather than MapManager maps.

Currently, map info (key type, value type, hist, and lhist args) is stored in 4 separate std::maps in RequiredResources. In future, we plan to add more info and will possibly need to access multiple items from a single place (typically key and value type). Therefore, it is much more efficient to aggregate everything in a single map (indexed by map name) so that we always need to do just one lookup.

Async map actions (print, clear, zero) now use maps from BpfBytecode instead of MapManager. This comes with two significant problems: - we do use map ids to identify maps in codegen anymore but we still need them for async actions, - some information used by async actions (e.g. key/value types) is not in BpfBytecode maps (instances of BpfMap). Since we want to keep the generated BPF programs decoupled from runtime as much as possible, we store the above information in RequiredResources since those are accessible by async actions. Part of the information is already there and this commit adds map id to MapInfo. In addition, we create a new mapping id->BpfMap in BpfBytecode to allow for efficient lookup of maps by their id. This is then used from async actions. Thanks to the chosen approach, instances of BpfMap can be fully populated by parsing the BPF program ELF and all other information is retrieved from RequiredResources. This also removes the last usages of MapManager and *Map classes so they will be removed in the following commit.

Now that map management has been completely moved to BpfBytecode, using BpfMap objects, we can remove the legacy MapManager and all of the related classes: IMap, Map, FakeMap. One problem that popped up is that now, required_resources.cpp only contains methods used by AOT. To avoid linker errors, we create a new target required_resources which is now linked from libaot only. Once we add methods required by libruntime to reqired_resources.cpp, we'll simply link against that target.

viktormalik · 2024-02-28T10:11:45Z

Rebased. Going to merge this to avoid more conflicts with open PRs.

viktormalik requested review from ajor, danobi and fbs as code owners February 9, 2024 08:13

viktormalik mentioned this pull request Feb 9, 2024

Improving map management #2911

Closed

5 tasks

github-advanced-security bot found potential problems Feb 9, 2024

View reviewed changes

src/bpftrace.cpp Fixed Show fixed Hide fixed

src/bpftrace.cpp Fixed Show fixed Hide fixed

ajor self-assigned this Feb 9, 2024

danobi self-assigned this Feb 9, 2024

viktormalik force-pushed the libbpf-maps branch from 5118ac7 to 03e9287 Compare February 12, 2024 13:39

github-advanced-security bot found potential problems Feb 12, 2024

View reviewed changes

src/bpftrace.cpp Fixed Show fixed Hide fixed

viktormalik force-pushed the libbpf-maps branch from 03e9287 to f5e423e Compare February 12, 2024 13:54

viktormalik force-pushed the libbpf-maps branch from f5e423e to 6b3a8b8 Compare February 14, 2024 12:57

ajor reviewed Feb 14, 2024

View reviewed changes

viktormalik force-pushed the libbpf-maps branch from 6b3a8b8 to bc82b0d Compare February 15, 2024 11:22

viktormalik force-pushed the libbpf-maps branch from bc82b0d to 8bb915b Compare February 15, 2024 15:24

viktormalik commented Feb 15, 2024

View reviewed changes

src/bpfbytecode.cpp Show resolved Hide resolved

ajor approved these changes Feb 15, 2024

View reviewed changes

src/bpfprogram.cpp Outdated Show resolved Hide resolved

src/bpfbytecode.cpp Show resolved Hide resolved

src/bpftrace.cpp Outdated Show resolved Hide resolved

src/bpftrace.cpp Outdated Show resolved Hide resolved

src/output.cpp Outdated Show resolved Hide resolved

viktormalik force-pushed the libbpf-maps branch from 8bb915b to 5d2de2c Compare February 15, 2024 19:30

viktormalik added the do-not-squash Do not squash PR commits label Feb 15, 2024

danobi reviewed Feb 15, 2024

View reviewed changes

src/bpfbytecode.h Show resolved Hide resolved

src/bpfmap.h Show resolved Hide resolved

src/bpfbytecode.cpp Outdated Show resolved Hide resolved

viktormalik force-pushed the libbpf-maps branch from 5d2de2c to 4ba6286 Compare February 16, 2024 05:48

danobi reviewed Feb 16, 2024

View reviewed changes

viktormalik force-pushed the libbpf-maps branch 2 times, most recently from ab31c85 to 78c3369 Compare February 20, 2024 07:38

viktormalik force-pushed the libbpf-maps branch from 78c3369 to a5f67f5 Compare February 20, 2024 08:22

viktormalik requested review from danobi and ajor February 20, 2024 08:51

danobi approved these changes Feb 20, 2024

View reviewed changes

danobi mentioned this pull request Feb 20, 2024

Set up clang-tidy in CI #2971

Open

viktormalik added 6 commits February 28, 2024 08:39

viktormalik force-pushed the libbpf-maps branch from a5f67f5 to c18f9d8 Compare February 28, 2024 08:35

viktormalik merged commit 651f25a into bpftrace:master Feb 28, 2024
19 checks passed

viktormalik deleted the libbpf-maps branch April 23, 2024 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move map management to `BpfBytecode` #2988

Move map management to `BpfBytecode` #2988

viktormalik commented Feb 9, 2024

viktormalik commented Feb 13, 2024

viktormalik commented Feb 14, 2024

ajor commented Feb 14, 2024

ajor left a comment

viktormalik commented Feb 15, 2024

viktormalik commented Feb 15, 2024

viktormalik commented Feb 15, 2024

ajor left a comment

viktormalik commented Feb 15, 2024

danobi left a comment

danobi left a comment

danobi Feb 16, 2024

viktormalik Feb 19, 2024

ajor Feb 19, 2024

viktormalik Feb 19, 2024

ajor Feb 19, 2024 •

edited

danobi Feb 19, 2024

viktormalik Feb 20, 2024

viktormalik commented Feb 28, 2024

		@@ -1,4 +1,4 @@
		add_library(aot aot.cpp)
		add_library(aot aot.cpp ${CMAKE_SOURCE_DIR}/src/required_resources.cpp)

Move map management to BpfBytecode #2988

Move map management to BpfBytecode #2988

Conversation

viktormalik commented Feb 9, 2024

Checklist

viktormalik commented Feb 13, 2024

viktormalik commented Feb 14, 2024

ajor commented Feb 14, 2024

ajor left a comment

Choose a reason for hiding this comment

viktormalik commented Feb 15, 2024

viktormalik commented Feb 15, 2024

viktormalik commented Feb 15, 2024

ajor left a comment

Choose a reason for hiding this comment

viktormalik commented Feb 15, 2024

danobi left a comment

Choose a reason for hiding this comment

danobi left a comment

Choose a reason for hiding this comment

danobi Feb 16, 2024

Choose a reason for hiding this comment

viktormalik Feb 19, 2024

Choose a reason for hiding this comment

ajor Feb 19, 2024

Choose a reason for hiding this comment

viktormalik Feb 19, 2024

Choose a reason for hiding this comment

ajor Feb 19, 2024 • edited

Choose a reason for hiding this comment

danobi Feb 19, 2024

Choose a reason for hiding this comment

viktormalik Feb 20, 2024

Choose a reason for hiding this comment

viktormalik commented Feb 28, 2024

Move map management to `BpfBytecode` #2988

Move map management to `BpfBytecode` #2988

ajor Feb 19, 2024 •

edited