Re: [PATCH bpf-next v3 3/4] libbpf: Add auto-pinning of maps when loading BPF objects

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sun, Oct 27, 2019 at 1:53 PM 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, to get automatic map pinning (and reuse) on load.
> The pinning type currently only supports a single PIN_BY_NAME mode, where
> each map will be pinned by its name in a path that can be overridden, but
> defaults to /sys/fs/bpf.
>
> 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() (including
> setting it to NULL to disable pinning of a particular map).
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/bpf_helpers.h |    6 ++
>  tools/lib/bpf/libbpf.c      |  142 ++++++++++++++++++++++++++++++++++++++++++-
>  tools/lib/bpf/libbpf.h      |   11 +++
>  3 files changed, 154 insertions(+), 5 deletions(-)
>

[...]

>
> -static int bpf_object__init_maps(struct bpf_object *obj, bool relaxed_maps)
> +static int bpf_object__build_map_pin_paths(struct bpf_object *obj,
> +                                          const char *path)
> +{
> +       struct bpf_map *map;
> +
> +       if (!path)
> +               path = "/sys/fs/bpf";
> +
> +       bpf_object__for_each_map(map, obj) {
> +               char buf[PATH_MAX];
> +               int err, len;
> +
> +               if (map->pinning != LIBBPF_PIN_BY_NAME)
> +                       continue;

still think it's better be done from map definition parsing code
instead of a separate path, which will ignore most of maps anyways (of
course by extracting this whole buffer creation logic into a
function).


> +
> +               len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
> +               if (len < 0)
> +                       return -EINVAL;
> +               else if (len >= PATH_MAX)

[...]

>         return 0;
>  }
>
> +static bool map_is_reuse_compat(const struct bpf_map *map,
> +                               int map_fd)

nit: this should fit on single line?

> +{
> +       struct bpf_map_info map_info = {};
> +       char msg[STRERR_BUFSIZE];
> +       __u32 map_info_len;
> +
> +       map_info_len = sizeof(map_info);
> +
> +       if (bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len)) {
> +               pr_warn("failed to get map info for map FD %d: %s\n",
> +                       map_fd, libbpf_strerror_r(errno, msg, sizeof(msg)));
> +               return false;
> +       }
> +
> +       return (map_info.type == map->def.type &&
> +               map_info.key_size == map->def.key_size &&
> +               map_info.value_size == map->def.value_size &&
> +               map_info.max_entries == map->def.max_entries &&
> +               map_info.map_flags == map->def.map_flags &&
> +               map_info.btf_key_type_id == map->btf_key_type_id &&
> +               map_info.btf_value_type_id == map->btf_value_type_id);

If map was pinned by older version of the same app, key and value type
id are probably gonna be different, even if the type definition itself
it correct. We probably shouldn't check that?

> +}
> +
> +static int
> +bpf_object__reuse_map(struct bpf_map *map)
> +{
> +       char *cp, errmsg[STRERR_BUFSIZE];
> +       int err, pin_fd;
> +
> +       pin_fd = bpf_obj_get(map->pin_path);
> +       if (pin_fd < 0) {
> +               if (errno == ENOENT) {
> +                       pr_debug("found no pinned map to reuse at '%s'\n",
> +                                map->pin_path);
> +                       return 0;
> +               }
> +
> +               cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
> +               pr_warn("couldn't retrieve pinned map '%s': %s\n",
> +                       map->pin_path, cp);
> +               return -errno;

store errno locally


> +       }
> +
> +       if (!map_is_reuse_compat(map, pin_fd)) {
> +               pr_warn("couldn't reuse pinned map at '%s': "
> +                       "parameter mismatch\n", map->pin_path);
> +               close(pin_fd);
> +               return -EINVAL;
> +       }
> +
> +       err = bpf_map__reuse_fd(map, pin_fd);
> +       if (err) {
> +               close(pin_fd);
> +               return err;
> +       }
> +       map->pinned = true;
> +       pr_debug("reused pinned map at '%s'\n", map->pin_path);
> +
> +       return 0;
> +}
> +

[...]

> +enum libbpf_pin_type {
> +       LIBBPF_PIN_NONE,
> +       /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
> +       LIBBPF_PIN_BY_NAME,
> +};
> +
>  LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);

pin_maps should take into account opts->auto_pin_path, shouldn't it?

Which is why I also think that auto_pin_path is bad name, because it's
not only for auto-pinning, it's a pinning root path, so something like
pin_root_path or just pin_root is better and less misleading name.



>  LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
>                                       const char *path);
>