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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Sun, Oct 27, 2019 at 5:04 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >>
>> >> 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 <toke@redhat.com>
>> >> ---
>> >
>> > 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.
>
>>
>> > Please add proper tests to test_progs, testing various modes and
>> > overrides.
>>
>> Can do.
>>
>> > 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! ;)
>>
>> > 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
>>
>> > 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?

We could, and I did actually try that. However, I think it is more
readable to have it be a separate step: init_user_btf_maps() parses the
map def, and after that is done we loop over things to build the pin
paths.

I'll send a v3 in a bit, you can see for yourself :)

-Toke