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

Andrii Nakryiko [off-list ref] writes:

On Sun, Oct 27, 2019 at 1:53 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

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

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

[...]

quoted

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

Hmm, okay, can do that. I think we should still store the actual value
of the 'pinning' attribute, though; and even have a getter for it. The
app may want to do something with that information instead of having to
infer it from map->pin_path. Certainly when we add other values of the
pinning attribute, but we may as well add the API to get the value
now...

quoted

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

[...]

quoted

        return 0;
 }

+static bool map_is_reuse_compat(const struct bpf_map *map,
+                               int map_fd)

nit: this should fit on single line?

quoted

+{
+       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?

Oh, I thought the type IDs would stay relatively stable. If not then I
agree that we shouldn't be checking them here. Will fix.

quoted

+}
+
+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

*shrugs* okay, if you insist...

quoted

+       }
+
+       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;
+}
+

[...]

quoted

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

I view auto_pin_path as something that is used specifically for the
automatic pinning based on the 'pinning' attribute. Any other use of
pinning is for custom use and the user can pass a custom pin path to
those functions.