Message-ID: <CAEf4BzbBmm3GfytbEtHwoD71p2XfuxuSYjhbb7rqPwUaYqvk7g@mail.gmail.com>
Date:   Thu, 24 Oct 2019 21:41:01 -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 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?

Please add proper tests to test_progs, testing various modes and overrides.

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. 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? ;)

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.


>  tools/lib/bpf/libbpf.c |  120 ++++++++++++++++++++++++++++++++++++++++++++++--
>  tools/lib/bpf/libbpf.h |    4 +-
>  2 files changed, 119 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 179c9911458d..e911760cd7ff 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1378,7 +1378,29 @@ static int build_pin_path(char *buf, size_t buf_len,
>         return len;
>  }
>
> -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)

shouldn't this whole thing take into account pinning attribute and not
do anything for maps that didn't request pinning?

> +{
> +       struct bpf_map *map;
> +       int err, len;
> +
> +       bpf_object__for_each_map(map, obj) {
> +               char buf[PATH_MAX];
> +               len = build_pin_path(buf, sizeof(buf), map,
> +                                    "/sys/fs/bpf", false);

"/sys/fs/bpf" should be overridable, this is why I'm insisting on
putting this pin_root_path override into open_opts, instead of
separate pin_opts. How all this was supposed to work, your approach
looks quite incoherent...

> +               if (len == 0)
> +                       continue;
> +               else if (len < 0)
> +                       return len;
> +
> +               err = bpf_map__set_pin_path(map, buf);
> +               if (err)
> +                       return err;
> +       }
> +       return 0;
> +}
> +
> +static int bpf_object__init_maps(struct bpf_object *obj, bool relaxed_maps,
> +                                bool auto_pin_maps)
>  {
>         bool strict = !relaxed_maps;
>         int err;
> @@ -1395,6 +1417,12 @@ static int bpf_object__init_maps(struct bpf_object *obj, bool relaxed_maps)
>         if (err)
>                 return err;
>
> +       if (auto_pin_maps) {

I don't think we need this option, unless proven otherwise. Let's do
this always. If application doesn't want auto-pinning, it shouldn't
specify .pinning = BY_NAME, or should bpf_map__set_pin_path(NULL)

> +               err = bpf_object__build_map_pin_paths(obj);
> +               if (err)
> +                       return err;
> +       }
> +
>         if (obj->nr_maps) {
>                 qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]),
>                       compare_bpf_map);
> @@ -1577,7 +1605,8 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj)
>         return 0;
>  }
>
> -static int bpf_object__elf_collect(struct bpf_object *obj, bool relaxed_maps)
> +static int bpf_object__elf_collect(struct bpf_object *obj, bool relaxed_maps,
> +                                  bool auto_pin_maps)
>  {
>         Elf *elf = obj->efile.elf;
>         GElf_Ehdr *ep = &obj->efile.ehdr;
> @@ -1712,7 +1741,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, bool relaxed_maps)
>         }
>         err = bpf_object__init_btf(obj, btf_data, btf_ext_data);
>         if (!err)
> -               err = bpf_object__init_maps(obj, relaxed_maps);
> +               err = bpf_object__init_maps(obj, relaxed_maps, auto_pin_maps);
>         if (!err)
>                 err = bpf_object__sanitize_and_load_btf(obj);
>         if (!err)
> @@ -2288,12 +2317,91 @@ bpf_object__create_maps(struct bpf_object *obj)
>                         }
>                 }
>
> +               if (map->pin_path) {
> +                       err = bpf_map__pin(map, NULL);
> +                       if (err)
> +                               pr_warning("failed to auto-pin map name '%s' at '%s'\n",
> +                                          map->name, map->pin_path);

this should be hard error

> +               }
> +
>                 pr_debug("created map %s: fd=%d\n", map->name, *pfd);
>         }
>
>         return 0;
>  }
>
> +static int check_map_compat(const struct bpf_map *map,
> +                           int map_fd)

super confusing set of return values, plus name doesn't really clarify
what's expected. Let's call it something like
is_pinned_map_compatible(), then we follow typical boolean-returning
convention: <0 - error, 0 - false, 1 - true. No confusion, no
guessing.

> +{
> +       struct bpf_map_info map_info = {};
> +       char msg[STRERR_BUFSIZE];
> +       __u32 map_info_len;
> +       int err;
> +
> +       map_info_len = sizeof(map_info);
> +       err = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len);
> +       if (err) {
> +               err = -errno;
> +               pr_warning("failed to get map info for map FD %d: %s\n",
> +                          map_fd, libbpf_strerror_r(err, msg, sizeof(msg)));
> +               return err;
> +       }
> +
> +       if (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)
> +               return 1;
> +
> +       return 0;
> +}
> +
> +static int
> +bpf_object__check_map_reuse(struct bpf_object *obj)


This is not a check, it is an attempt to reuse, so just bpf_object__reuse_maps?

> +{
> +       char *cp, errmsg[STRERR_BUFSIZE];
> +       struct bpf_map *map;
> +       int err;
> +
> +       bpf_object__for_each_map(map, obj) {
> +               int pin_fd;
> +
> +               if (!map->pin_path)
> +                       continue;
> +
> +               pin_fd = bpf_obj_get(map->pin_path);
> +               if (pin_fd < 0) {
> +                       if (errno == ENOENT)
> +                               continue;
> +
> +                       cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
> +                       pr_warning("Couldn't retrieve pinned map '%s': %s\n",
> +                                  map->pin_path, cp);
> +                       return pin_fd;
> +               }
> +
> +               if (check_map_compat(map, pin_fd)) {
> +                       pr_warning("Couldn't reuse pinned map at '%s': "
> +                                  "parameter mismatch\n", map->pin_path);

Please don't split strings.

Also, prevalent form of log messages is to start them with lower-cased
words, please keep consistency.




> +                       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;
> +}
> +
>  static int
>  check_btf_ext_reloc_err(struct bpf_program *prog, int err,
>                         void *btf_prog_info, const char *info_name)
> @@ -3664,6 +3772,7 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>  {
>         struct bpf_object *obj;
>         const char *obj_name;
> +       bool auto_pin_maps;
>         char tmp_name[64];
>         bool relaxed_maps;
>         int err;
> @@ -3695,11 +3804,13 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>
>         obj->relaxed_core_relocs = OPTS_GET(opts, relaxed_core_relocs, false);
>         relaxed_maps = OPTS_GET(opts, relaxed_maps, false);
> +       auto_pin_maps = OPTS_GET(opts, auto_pin_maps, true);
>
>         CHECK_ERR(bpf_object__elf_init(obj), err, out);
>         CHECK_ERR(bpf_object__check_endianness(obj), err, out);
>         CHECK_ERR(bpf_object__probe_caps(obj), err, out);
> -       CHECK_ERR(bpf_object__elf_collect(obj, relaxed_maps), err, out);
> +       CHECK_ERR(bpf_object__elf_collect(obj, relaxed_maps, auto_pin_maps),
> +                 err, out);
>         CHECK_ERR(bpf_object__collect_reloc(obj), err, out);
>
>         bpf_object__elf_finish(obj);
> @@ -3811,6 +3922,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>
>         obj->loaded = true;
>
> +       CHECK_ERR(bpf_object__check_map_reuse(obj), err, out);
>         CHECK_ERR(bpf_object__create_maps(obj), err, out);
>         CHECK_ERR(bpf_object__relocate(obj, attr->target_btf_path), err, out);
>         CHECK_ERR(bpf_object__load_progs(obj, attr->log_level), err, out);
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 26a4ed3856e7..d492920fedb3 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -98,8 +98,10 @@ struct bpf_object_open_opts {
>         bool relaxed_maps;
>         /* process CO-RE relocations non-strictly, allowing them to fail */
>         bool relaxed_core_relocs;
> +       /* auto-pin maps with 'pinning' attribute set? */
> +       bool auto_pin_maps;
>  };
> -#define bpf_object_open_opts__last_field relaxed_core_relocs
> +#define bpf_object_open_opts__last_field auto_pin_maps
>
>  LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
>  LIBBPF_API struct bpf_object *
>

Powered by blists - more mailing lists