Re: [PATCH bpf-next v2 4/4] libbpf: Add option to auto-pin maps when opening BPF object

On Sun, Oct 27, 2019 at 5:04 AM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

With the functions added in previous commits that can automatically pin
maps based on their 'pinning' setting, we can support auto-pinning of maps
by the simple setting of an option to bpf_object__open.

Since auto-pinning only does something if any maps actually have a
'pinning' BTF attribute set, we default the new option to enabled, on the
assumption that seamless pinning is what most callers want.

When a map has a pin_path set at load time, libbpf will compare the map
pinned at that location (if any), and if the attributes match, will re-use
that map instead of creating a new one. If no existing map is found, the
newly created map will instead be pinned at the location.

Programs wanting to customise the pinning can override the pinning paths
using bpf_map__set_pin_path() before calling bpf_object__load().

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

How have you tested this? From reading the code, all the maps will be
pinned irregardless of their .pinning setting?

No, build_pin_path() checks map->pinning :)

subtle... build_pin_path() definitely doesn't imply that it's a "maybe
build pin path?", but see below for pin_path setting.

quoted

Please add proper tests to test_progs, testing various modes and
overrides.

Can do.

quoted

You keep trying to add more and more knobs :) Please stop doing that,
even if we have a good mechanism for extensibility, it doesn't mean we
need to increase a proliferation of options.

But I like options! ;)

quoted

Each option has to be tested. In current version of your patches, you
have something like 4 or 5 different knobs, do you really want to
write tests testing each of them? ;)

Heh, I guess I can cut down the number of options to the number of tests :P

quoted

Another high-level feedback. I think having separate passes over all
maps (build_map_pin_paths, reuse, then we already have create_maps) is
actually making everything more verbose and harder to extend. I'm
thinking about all these as sub-steps of map creation. Can you please
try refactoring so all these steps are happening per each map in one
place: if map needs to be pinned, check if it can be reused, if not -
create it. This actually will allow to handle races better, because
you will be able to retry easily, while if it's all spread in
independent passes, it becomes much harder. Please consider that.

We'll need at least two passes: set pin_path on open, and check reuse /
create / pin on load. Don't have any objections to consolidating the
other passes into create_maps; will fix, along with your comments below.

for BTF-defined maps, can't we just set a pin_path right when we are
reading map definition? With that, we won't even need to store
.pinning field. Would that work?

-Toke