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 [off-list ref] wrote:

From: Toke Høiland-Jørgensen <redacted>

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 <redacted>
---

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.

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...

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)

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.