Open astoycos opened 1 year ago
On the eBPF side itself users were also provided the LIBBPF_PIN_BY_NAME flag.
This is not true. It's only required if the BPF-side wants to indicate to the loader that it SHOULD be pinned. Userland can pin it regardless of whether the flag is present or not, but it won't happen automagically.
Today the API(s) has been cleaned up and works like the following but IMO is still broken:
Broken how?
The examples you've given are fixed on .map_pin_path
, which only comes into play when used with LIBBPF_PIN_BY_NAME
, but this only one of the may ways in which you can pin maps.
See also: https://docs.aya-rs.dev/aya/maps/enum.map#method.pin
The interactions you describe between the root pin path + the LIBBPF_PIN_BY_NAME flag are, as I recall, exactly the same as you get from libbpf so I'm 👎 on changing anything there.
Specify a map pin path at load time allowing the user to pin a map by name OR via a direct arbitrary path, always re-creating the map.
This doesn't make sense to me. How can you always re-create the map if there is always something at the pinned location?
I'm not sure that doing this in BpfLoader makes sense either - e.g .map_pin_path("foo", "/path/to/foo").map_pin_path("bar", "/path/to/bar")
. I'll touch on this again later.
Specify that all maps should be re-created and pinned by name at load time.
What do you do when you have a conflict on pin path? E.g one or more maps already exist?
Specify that all maps should be re-used from pins by name at load time, failing if they don't exist. (notice this is takes explicit user intent rather than being bundled into 3.)
I mean sure, but I think this gets weird pretty quickly.
Specify a map pin path at load time allowing the user to re-use a map pin by name OR re-use a pin from a driect arbitrary path, always re-using the map and failing if it doesn't already exist (OR possibly from a previously created map see
I'm ok with this as a use-case, but I don't think this should be the "default" behaviour.
...
Pinning workflows are pretty advanced BPF stuff, and I don't think we should touch the existing user experience too much.
I do however think that part of the problem is in BpfLoader::load
- we've bumped up against multiple use-cases for splitting map creation and loading the bytecode into the kernel and I think that should be the focus of the work you're doing.
For example, look at what you can do with libbpf:
bpf_object__open_*
bpf_object__pin_maps
(I imagine that will exclude read-only maps like .rodata and .bss etc...)bpf_map__pin
- I'd assume the root path is inherited from the the original open call, but you can also provide a pathbpf_map__set_pin_path
to change the pin path on a per-map-basisbpf_map__pin
bpf_map__set_autocreate
disables a map from being autocreated when you load it into the kernel.(Some investigation required to know whether it appends name to the path
parameter in all cases)
Having equivalent APIs in Aya to expose the same features would be great.
TL:DR
I think that where you need to focus on this here. Specifically:
BpfLoader::new()
into BpfLoader::new(bytes)
BpfLoader
should be able to manipulate the parsed bytecode - that should provide better experience for things like set_global
or marking programs as bpf_prog_type_ext
, manipulating maps, creating your own maps from userspace etc...BpfLoader::load()
is still the last call in the chain, and yields an aya::Bpf... it just doesn't take a path to the bytecode anymore.With ☝️ done, manipulating stuff before loading can now fail early - i.e we know that global variable "bar" doesn't exist... or that map "foo" isn't there... etc...
Oh just a note, all of the weird and wonderful pinning use-cases you mentioned can be expressed as one-or-more calls to BpfLoader
APIs before calling BpfLoader::load()
. I'd much rather we did that than, say, adding lots of options like always_reuse_maps_from_pins_or_else_error(true)
etc...
The Problem
Originally (since I've been a contributor) we we're only given a single overloaded API by aya regarding bpf maps at load time, i.e
.map_pin_path()
.On the eBPF side itself users were also provided the
LIBBPF_PIN_BY_NAME
flag.Originally (before some recent patch sets)
.map_pin_path()
+ LIBBPF_PIN_BY_NAME actually resulted in 2 functions:LIBBPF_MAP_PIN_BY_NAME
flag.Current Behavior
Today the API(s) has been cleaned up and works like the following but IMO is still broken:
.map_pin_path(PATH)
IS NOT provided and LIBBPF_PIN_BY_NAME IS NOT set, the map is created and not pinned anywhere..map_pin_path(PATH)
IS provided but LIBBPF_PIN_BY_NAME IS NOT set, the map is created and not pinned anywhere..map_pin_path(PATH)
IS NOT provided and LIBBPF_PIN_BY_NAME IS set, the map is created and pinned by name at/sys/fs/bpf
, pins are re-used if they exist.map_pin_path(PATH)
IS provided and LIBBPF_PIN_BY_NAME IS set, the map is created and pinned by name at PATH, pins are re-used if they existMoving forward (our supported/documented use cases)
I propose adding/fixing the APIs to behave in a more predictable manner while also at a high level allowing users to:
LIBBPF_PIN_BY_NAME
to work if:LIBBPF_PIN_BY_NAME
is set.I would like to align on these use cases BEFORE moving forward with the API design.