Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 1:37 PM Anton Protopopov
<a.s.protopopov@xxxxxxxxx> wrote:
>
> ÐÐ, 8 ÐÑÐ. 2019 Ð. Ð 13:54, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>:
> >
> > On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> > >
> > > On 07/05/2019 10:44 PM, Anton Protopopov wrote:
> > > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in
> > > > an object by maps pinned to a directory provided in the path argument.  Namely,
> > > > each map M in the object will be replaced by a map pinned to path/M.name.
> > > >
> > > > Signed-off-by: Anton Protopopov <a.s.protopopov@xxxxxxxxx>
> > > > ---
> > > >  tools/lib/bpf/libbpf.c   | 34 ++++++++++++++++++++++++++++++++++
> > > >  tools/lib/bpf/libbpf.h   |  2 ++
> > > >  tools/lib/bpf/libbpf.map |  1 +
> > > >  3 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index 4907997289e9..84c9e8f7bfd3 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
> > > >       return 0;
> > > >  }
> > > >
> > > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path)
> >
> > As is, bpf_object__reuse_maps() can be easily implemented by user
> > applications, as it's only using public libbpf APIs, so I'm not 100%
> > sure we need to add method like that to libbpf.
>
> The bpf_object__reuse_maps() can definitely be implemented by user
> applications, however, to use it a user also needs to re-implement the
> bpf_prog_load_xattr funciton, so it seemed to me that adding this
> functionality to the library is a better way.

I'm still not convinced. Looking at bpf_prog_load_xattr, I think some
of what it's doing should be part of bpf_object__object_xattr anyway
(all the expected type setting for programs).

Besides that, there isn't much more than just bpf_object__open and
bpf_object__load, to be honest. By doing open and load explicitly,
user gets an opportunity to do whatever adjustment they need: reuse
maps, adjust map sizes, etc. So I think we should improve
bpf_object__open to "guess" program attach types and add map
definition flags to allow reuse declaratively.


>
> >
> > > > +{
> > > > +     struct bpf_map *map;
> > > > +
> > > > +     if (!obj)
> > > > +             return -ENOENT;
> > > > +
> > > > +     if (!path)
> > > > +             return -EINVAL;
> > > > +
> > > > +     bpf_object__for_each_map(map, obj) {
> > > > +             int len, err;
> > > > +             int pinned_map_fd;
> > > > +             char buf[PATH_MAX];
> > >
> > > We'd need to skip the case of bpf_map__is_internal(map) since they are always
> > > recreated for the given object.
> > >
> > > > +             len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
> > > > +             if (len < 0) {
> > > > +                     return -EINVAL;
> > > > +             } else if (len >= PATH_MAX) {
> > > > +                     return -ENAMETOOLONG;
> > > > +             }
> > > > +
> > > > +             pinned_map_fd = bpf_obj_get(buf);
> > > > +             if (pinned_map_fd < 0)
> > > > +                     return pinned_map_fd;
> > >
> > > Should we rather have a new map definition attribute that tells to reuse
> > > the map if it's pinned in bpf fs, and if not, we create it and later on
> > > pin it? This is what iproute2 is doing and which we're making use of heavily.
> >
> > I'd like something like that as well. This would play nicely with
> > recently added BTF-defined maps as well.
> >
> > I think it should be not just pin/don't pin flag, but rather pinning
> > strategy, to accommodate various typical strategies of handling maps
> > that are already pinned. So something like this:
> >
> > 1. BPF_PIN_NOTHING - default, don't pin;
> > 2. BPF_PIN_EXCLUSIVE - pin, but if map is already pinned - fail;
> > 3. BPF_PIN_SET - pin; if existing map exists, reset its state to be
> > exact state of object's map;
> > 4. BPF_PIN_MERGE - pin, if map exists, fill in NULL entries only (this
> > is how Cilium is pinning PROG_ARRAY maps, if I understand correctly);
> > 5. BPF_PIN_MERGE_OVERWRITE - pin, if map exists, overwrite non-NULL values.
> >
> > This list is only for illustrative purposes, ideally people that have
> > a lot of experience using pinning for real-world use cases would chime
> > in on what strategies are useful and make sense.
>
> My case was simply to reuse existing maps when reloading a program.
> Does it make sense for you to add only the simplest cases of listed above?

Of course, it's enum, so we can start with few clearly useful ones and
then expand more if we ever have a need. But I think we still need a
bit wider discussion and let people who use pinning to chime in.

>
> Also, libbpf doesn't use standard naming conventions for pinning maps.

We talked about this in another thread related to BTF-defined maps. I
think the way to go with this is to actually define a default pinning
root path, but allow to override it on bpf_object__open, if user needs
a different one.

> Does it make sense to provide a list of already open maps to the
> bpf_prog_load_xattr function as an attribute? In this case a user
> can execute his own policy on pinning, but still will have an option
> to reuse, reset, and merge maps.

As explained above, I don't think there isn't much added value in
bpf_prog_load, so I'd advise to just switch to explicit
bpf_object__open + bpf_object__load and get maximum control and
flexibility.

>
> >
> > > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps
> > > too limiting for a generic API as new version of an object file may contain
> > > new maps which are not yet present in bpf fs at that point.
> > >
> > > > +             err = bpf_map__reuse_fd(map, pinned_map_fd);
> > > > +             if (err)
> > > > +                     return err;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
> > > >  {
> > > >       struct bpf_program *prog;
> > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > > index d639f47e3110..7fe465a1be76 100644
> > > > --- a/tools/lib/bpf/libbpf.h
> > > > +++ b/tools/lib/bpf/libbpf.h
> > > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
> > > >  LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);
> > > >  LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
> > > >                                     const char *path);
> > > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj,
> > > > +                                   const char *path);
> > > >  LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
> > > >                                       const char *path);
> > > >  LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
> > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > > index 2c6d835620d2..66a30be6696c 100644
> > > > --- a/tools/lib/bpf/libbpf.map
> > > > +++ b/tools/lib/bpf/libbpf.map
> > > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 {
> > > >               btf_dump__new;
> > > >               btf__parse_elf;
> > > >               bpf_object__load_xattr;
> > > > +             bpf_object__reuse_maps;
> > > >               libbpf_num_possible_cpus;
> > > >  } LIBBPF_0.0.3;
> > > >
> > >