Re: [PATCH bpf-next v3 2/4] libbpf: Store map pin path and status in struct bpf_map

On Tue, Oct 29, 2019 at 2:01 AM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

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>

Support storing and setting a pin path in struct bpf_map, which can be used
for automatic pinning. Also store the pin status so we can avoid attempts
to re-pin a map that has already been pinned (or reused from a previous
pinning).

Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 tools/lib/bpf/libbpf.c   |  115 ++++++++++++++++++++++++++++++++++++----------
 tools/lib/bpf/libbpf.h   |    3 +
 tools/lib/bpf/libbpf.map |    3 +
 3 files changed, 97 insertions(+), 24 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ce5ef3ddd263..eb1c5e6ad4a3 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -226,6 +226,8 @@ struct bpf_map {
        void *priv;
        bpf_map_clear_priv_t clear_priv;
        enum libbpf_map_type libbpf_type;
+       char *pin_path;
+       bool pinned;
 };

 struct bpf_secdata {

@@ -4025,47 +4027,118 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
        char *cp, errmsg[STRERR_BUFSIZE];
        int err;

-       err = check_path(path);
-       if (err)
-               return err;
-
        if (map == NULL) {
                pr_warn("invalid map pointer\n");
                return -EINVAL;
        }

-       if (bpf_obj_pin(map->fd, path)) {
-               cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
-               pr_warn("failed to pin map: %s\n", cp);
-               return -errno;
+       if (map->pinned) {
+               pr_warn("map already pinned\n");

it would be helpful to print the name of the map, otherwise user will
have to guess

Well, the existing error message didn't include the map name, so I was
just being consistent. But sure I can change it (and the old message as
well).

quoted

quoted

+               return -EEXIST;
+       }
+
+       if (path && map->pin_path && strcmp(path, map->pin_path)) {
+               pr_warn("map already has pin path '%s' different from '%s'\n",
+                       map->pin_path, path);

here pin_path probably would be unique enough, but for consistency we
might want to print map name as well

quoted

+               return -EINVAL;
+       }
+
+       if (!map->pin_path && !path) {
+               pr_warn("missing pin path\n");

and here?

quoted

+               return -EINVAL;
        }

-       pr_debug("pinned map '%s'\n", path);
+       if (!map->pin_path) {
+               map->pin_path = strdup(path);
+               if (!map->pin_path) {
+                       err = -errno;
+                       goto out_err;
+               }
+       }

There is a bit of repetition of if conditions, based on whether we
have map->pin_path set (which is the most critical piece we care
about), so that makes it a bit harder to follow what's going on. How
about this structure, would it make a bit clearer what the error
conditions are? Not insisting, though.

if (map->pin_path) {
  if (path && strcmp(...))
    bad, exit
else { /* no pin_path */
  if (!path)
    very bad, exit
  map->pin_path = strdup(..)
  if (!map->pin_path)
    also bad, exit
}

Hmm, yeah, this may be better...

quoted

quoted

+
+       err = check_path(map->pin_path);
+       if (err)
+               return err;
+

[...]

quoted

+int bpf_map__set_pin_path(struct bpf_map *map, const char *path)
+{
+       char *old = map->pin_path, *new;
+
+       if (path) {
+               new = strdup(path);
+               if (!new)
+                       return -errno;
+       } else {
+               new = NULL;
+       }
+
+       map->pin_path = new;
+       if (old)
+               free(old);

you don't really need old, just free map->pin_path before setting it
to new. Also assigning new = NULL will simplify if above.

Right, will fix.

quoted

quoted

+
+       return 0;
+}
+
+const char *bpf_map__get_pin_path(struct bpf_map *map)
+{
+       return map->pin_path;
+}
+
+bool bpf_map__is_pinned(struct bpf_map *map)
+{
+       return map->pinned;
+}
+
 int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 {
        struct bpf_map *map;

@@ -4106,17 +4179,10 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)

I might have missed something the change in some other patch, but
shouldn't pin_maps ignore already pinned maps? Otherwise we'll be
generating unnecessary warnings?

Well, in the previous version this was in one of the options you didn't
like. If I just change pin_maps() unconditionally, that will be a change
in behaviour in an existing API. So I figured it would be better to
leave this as-is. I don't think this function is really useful along
with the auto-pinning anyway. If you're pinning all maps, why use
auto-pinning? And if you want to do something custom to all the
non-pinned maps you'd probably iterate through them yourself anyway and
can react appropriately?

Auto-pinned maps didn't exist before, so interaction between
auto-pinned and explicitly pinned maps is a new behavior.

With current code using explicit pin_maps and auto-pinned maps is
impossible, or am I missing something? While admittedly scenarios in
which you'll have to use explicit bpf_object__pin_maps() while you
have auto-pinned maps and bpf_map__set_pin_path() are quite exotic
(e.g., auto-pin some maps at default path and pin all the rest at some
other custom root), I think we should still try to make existing APIs
combinable in some sane way.

The only downside of ignoring already pinned maps is that while
previously calling pin_maps() twice in a row would fail fails second
time, now the second pin_maps() will be a noop. I think that's benign
and acceptable change in behavior? WDYT?

quoted

quoted

 err_unpin_maps:
        while ((map = bpf_map__prev(map, obj))) {
-               char buf[PATH_MAX];
-               int len;
-
-               len = snprintf(buf, PATH_MAX, "%s/%s", path,
-                              bpf_map__name(map));
-               if (len < 0)
-                       continue;
-               else if (len >= PATH_MAX)
+               if (!map->pin_path)
                        continue;

-               bpf_map__unpin(map, buf);
+               bpf_map__unpin(map, NULL);

so this will unpin auto-pinned maps (from BTF-defined maps). Is that
the desired behavior? I guess it might be ok (if you can't pin all of
your maps, you should probably clean all of them up?), but just
bringing it up.

Yeah, I realise that. Not entirely sure it's the right thing to do, but
there not really any way to disambiguate how the map was pinned; unless
we want to add another state field just for that? So I guess it's either
"don't do any cleanup" or just "unpin everything". And since I don't
think it'll be terribly useful to combine the use of this function with
auto-pinning anyway, I think it's probably fine to just unpin everything
here?

Yeah, I think all-or-nothing regarding pinning is ok behavior. It
would be strange to have BPF application which is fine with only some
of maps to be pinned.

-Toke