Re: [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations

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

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

> On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> This adds support to libbpf for setting map pinning information as part of
>> the BTF map declaration. We introduce a new pair of functions to pin and
>> unpin maps based on this setting, as well as a getter and setter function
>> for the pin information that callers can use after map load.
>>
>> The pin_type supports two modes: LOCAL pinning, which requires the caller
>> to set a pin path using bpf_object_pin_opts, and a global mode, where the
>> path can still be overridden, but defaults to /sys/fs/bpf. This is inspired
>> by the two modes supported by the iproute2 map definitions. In particular,
>> it should be possible to express the current iproute2 operating mode in
>> terms of the options introduced here.
>>
>> The new pin functions will skip any maps that do not have a pinning type
>> set, unless the 'override_type' option is set, in which case all maps will
>> be pinning using the pin type set in that option. This also makes it
>> possible to express the old pin_maps and unpin_maps functions in terms of
>> the new option-based functions.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> So few high-level thoughts.
>
> 1. I'd start with just NONE and GLOBAL as two pinning modes. It might
> be worth-while to name GLOBAL something different just to specify that
> it is just pinning, either to default /sys/fs/bpf root or some other
> user-provided root path.
> 1a. LOCAL seems to behave exactly like GLOBAL, just uses separate
> option for a path. So we effectively have two GLOBAL modes, one with
> default (but overrideable) /sys/fs/bpf, another with user-provided
> mandatory path. The distinction seem rather small and arbitrary.
> What's the use case?

Supporting iproute2, mostly :)

Don't terribly mind dropping LOCAL, though; I don't have any particular
use case in mind for it myself.

> 2. When is pin type override useful? Either specify it once
> declaratively in map definition, or just do pinning programmatically?

Dunno if it's really useful, actually. 

> 3. I think we should make pinning path override into
> bpf_object_open_opts and keep bpf_object__pin_maps simple. We are
> probably going to make map pinning/sharing automatic anyway, so that
> will need to happen as part of either open or load operation.

I actually started with just writing automatic map pinning logic for
open(), but found myself re-implementing most of the logic in map_pin().
So figured I might as well expose it to that as well.

For open/load I think the logic should be that we parse the pinning
attribute on open and set map->pin_path from that. Then load() looks at
pin_path and does the reuse/create dance. That way, an application can
set its own pin_paths between open and load to support legacy formats
(like iproute2 needs to).

> 4. Once pinned, map knows its pinned path, just use that, I don't see
> any reasonable use case where you'd want to override path just for
> unpinning.

Well, unpinning may need to re-construct the pin path. E.g.,
applications that exit after loading and are re-run after unloading,
such as iproute2, probably want to be able to unpin maps. Unfortunately
I don't think there is a way to get the pin path(s) of an object from
the kernel, though, is there? That would be kinda neat for implementing
something like `ip link set dev eth0 xdp off unpin`.

> Does it make sense?
>
>>  tools/lib/bpf/bpf_helpers.h |    8 +++
>>  tools/lib/bpf/libbpf.c      |  123 ++++++++++++++++++++++++++++++++++++-------
>>  tools/lib/bpf/libbpf.h      |   33 ++++++++++++
>>  tools/lib/bpf/libbpf.map    |    4 +
>>  4 files changed, 148 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>> index 2203595f38c3..a23cf55d41b1 100644
>> --- a/tools/lib/bpf/bpf_helpers.h
>> +++ b/tools/lib/bpf/bpf_helpers.h
>> @@ -38,4 +38,12 @@ struct bpf_map_def {
>>         unsigned int map_flags;
>>  };
>>
>> +enum libbpf_pin_type {
>> +       LIBBPF_PIN_NONE,
>> +       /* PIN_LOCAL: pin maps by name in path specified by caller */
>> +       LIBBPF_PIN_LOCAL,
>
> Daniel mentioned in previous discussions that LOCAL mode is never
> used. I'd like to avoid supporting unnecessary stuff. Is it really
> useful?

Oh, he did? In that case, let's definitely get rid of it :)

>> +       /* PIN_GLOBAL: pin maps by name in global path (/sys/fs/bpf by default) */
>> +       LIBBPF_PIN_GLOBAL,
>> +};
>> +
>>  #endif
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index b4fdd8ee3bbd..aea3916de341 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -226,6 +226,7 @@ struct bpf_map {
>>         void *priv;
>>         bpf_map_clear_priv_t clear_priv;
>>         enum libbpf_map_type libbpf_type;
>> +       enum libbpf_pin_type pinning;
>>         char *pin_path;
>>  };
>>
>> @@ -1270,6 +1271,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
>>                         }
>>                         map->def.value_size = sz;
>>                         map->btf_value_type_id = t->type;
>> +               } else if (strcmp(name, "pinning") == 0) {
>> +                       __u32 val;
>> +
>> +                       if (!get_map_field_int(map_name, obj->btf, def, m,
>> +                                              &val))
>> +                               return -EINVAL;
>> +                       pr_debug("map '%s': found pinning = %u.\n",
>> +                                map_name, val);
>> +
>> +                       if (val && val != LIBBPF_PIN_LOCAL &&
>> +                           val != LIBBPF_PIN_GLOBAL) {
>
> let's write out LIBBPF_PIN_NONE explicitly, instead of just `val`?

OK.

>> +                               pr_warning("map '%s': invalid pinning value %u.\n",
>> +                                          map_name, val);
>> +                               return -EINVAL;
>> +                       }
>> +                       map->pinning = val;
>>                 } else {
>>                         if (strict) {
>>                                 pr_warning("map '%s': unknown field '%s'.\n",
>> @@ -4055,10 +4072,51 @@ int bpf_map__unpin(struct bpf_map *map, const char *path)
>>         return 0;
>>  }
>>
>> -int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
>> +static int get_pin_path(char *buf, size_t buf_len,
>> +                       struct bpf_map *map, struct bpf_object_pin_opts *opts,
>> +                       bool mkdir)
>> +{
>> +       enum libbpf_pin_type type;
>> +       const char *path;
>> +       int err, len;
>> +
>> +       type = OPTS_GET(opts, override_type, 0) ?: map->pinning;
>> +
>> +       if (type == LIBBPF_PIN_GLOBAL) {
>> +               path = OPTS_GET(opts, path_global, NULL);
>> +               if (!path)
>> +                       path = "/sys/fs/bpf";
>> +       } else if (type == LIBBPF_PIN_LOCAL) {
>> +               path = OPTS_GET(opts, path_local, NULL);
>> +               if (!path) {
>> +                       pr_warning("map '%s' set pinning to PIN_LOCAL, "
>> +                                  "but no local path provided. Skipping.\n",
>> +                                  bpf_map__name(map));
>> +                       return 0;
>> +               }
>> +       } else {
>> +               return 0;
>> +       }
>> +
>> +       if (mkdir) {
>> +               err = make_dir(path);
>> +               if (err)
>> +                       return err;
>> +       }
>> +
>> +       len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map));
>> +       if (len < 0)
>> +               return -EINVAL;
>> +       else if (len >= buf_len)
>> +               return -ENAMETOOLONG;
>> +       return len;
>> +}
>> +
>> +int bpf_object__pin_maps_opts(struct bpf_object *obj,
>> +                             struct bpf_object_pin_opts *opts)
>>  {
>>         struct bpf_map *map;
>> -       int err;
>> +       int err, len;
>>
>>         if (!obj)
>>                 return -ENOENT;
>> @@ -4068,21 +4126,17 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
>>                 return -ENOENT;
>>         }
>>
>> -       err = make_dir(path);
>> -       if (err)
>> -               return err;
>> +       if (!OPTS_VALID(opts, bpf_object_pin_opts))
>> +               return -EINVAL;
>>
>>         bpf_object__for_each_map(map, obj) {
>>                 char buf[PATH_MAX];
>> -               int len;
>>
>> -               len = snprintf(buf, PATH_MAX, "%s/%s", path,
>> -                              bpf_map__name(map));
>> -               if (len < 0) {
>> -                       err = -EINVAL;
>> -                       goto err_unpin_maps;
>> -               } else if (len >= PATH_MAX) {
>> -                       err = -ENAMETOOLONG;
>> +               len = get_pin_path(buf, PATH_MAX, map, opts, true);
>> +               if (len == 0) {
>> +                       continue;
>> +               } else if (len < 0) {
>> +                       err = len;
>>                         goto err_unpin_maps;
>>                 }
>>
>> @@ -4104,7 +4158,16 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
>>         return err;
>>  }
>>
>> -int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
>> +int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
>> +{
>> +       LIBBPF_OPTS(bpf_object_pin_opts, opts,
>> +                   .path_global = path,
>> +                   .override_type = LIBBPF_PIN_GLOBAL);
>
> style nit: extra line between declaration and statements
>
>> +       return bpf_object__pin_maps_opts(obj, &opts);
>> +}
>> +
>> +int bpf_object__unpin_maps_opts(struct bpf_object *obj,
>> +                             struct bpf_object_pin_opts *opts)
>>  {
>>         struct bpf_map *map;
>>         int err;
>> @@ -4112,16 +4175,18 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
>>         if (!obj)
>>                 return -ENOENT;
>>
>> +       if (!OPTS_VALID(opts, bpf_object_pin_opts))
>> +               return -EINVAL;
>
> specifying pin options for unpin operation looks cumbersome. We know
> the pinned path, just use that and keep unpinning simple?

You are right, but see above re: recreating pin paths on re-run.

-Toke