Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <cover.1562359091.git.a.s.protopopov@gmail.com>
 <e183c0af99056f8ea4de06acb358ace7f3a3d6ae.1562359091.git.a.s.protopopov@gmail.com>
 <734dd45a-95b0-a7fd-9e1d-0535ef4d3e12@iogearbox.net> <CAEf4BzaGGVv2z8jB8MnT7=gnn4nG0cp7DGYxfnnnpohOT=ujCA@mail.gmail.com>
In-Reply-To: <CAEf4BzaGGVv2z8jB8MnT7=gnn4nG0cp7DGYxfnnnpohOT=ujCA@mail.gmail.com>
From:   Anton Protopopov <a.s.protopopov@gmail.com>
Date:   Mon, 8 Jul 2019 16:37:42 -0400
Message-ID: <CAGn_itw=BqWXn7ibg6M7j4r2T5CMo0paBhBoQQv7b+x7D2g2ww@mail.gmail.com>
Subject: Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     Daniel Borkmann <daniel@iogearbox.net>,
        Alexei Starovoitov <ast@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Andrii Nakryiko <andriin@fb.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=D0=BF=D0=BD, 8 =D0=B8=D1=8E=D0=BB. 2019 =D0=B3. =D0=B2 13:54, Andrii Nakry=
iko <andrii.nakryiko@gmail.com>:
>
> On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@iogearbox.net> wro=
te:
> >
> > On 07/05/2019 10:44 PM, Anton Protopopov wrote:
> > > Add a new API bpf_object__reuse_maps() which can be used to replace a=
ll 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.n=
ame.
> > >
> > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > > ---
> > >  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.

>
> > > +{
> > > +     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 =3D snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map_=
_name(map));
> > > +             if (len < 0) {
> > > +                     return -EINVAL;
> > > +             } else if (len >=3D PATH_MAX) {
> > > +                     return -ENAMETOOLONG;
> > > +             }
> > > +
> > > +             pinned_map_fd =3D 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 reus=
e
> > 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 he=
avily.
>
> 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 value=
s.
>
> 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?

Also, libbpf doesn't use standard naming conventions for pinning maps.
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.

>
> > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perha=
ps
> > too limiting for a generic API as new version of an object file may con=
tain
> > new maps which are not yet present in bpf fs at that point.
> >
> > > +             err =3D 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 *pat=
h)
> > >  {
> > >       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_ob=
ject *obj, const char *name,
> > >  LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const ch=
ar *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;
> > >
> >