Message-ID: <CAEf4Bzbn-wFJdhn5DCss8J4d7HNpHjUTrGKQqppv+ykjVAqMCA@mail.gmail.com>
Date:   Sun, 27 Oct 2019 13:12:21 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        David Miller <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: 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 <toke@...hat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@...il.com> writes:
>
> > On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@...hat.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@...hat.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?

>
> -Toke

Powered by blists - more mailing lists