[PATCH bpf-next v2 0/4] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute

[PATCH bpf-next v2 0/4] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute

[Toke Høiland-Jørgensen]

This series adds support to libbpf for reading 'pinning' settings from BTF-based
map definitions. It introduces a new bpf_object__pin_maps_opts() function which
accepts options with the new options struct definition style. This allows the
caller to configure the pinning path, and to use both old-style "forced pinning"
where all defined maps are pinned, and the new pinning that is based on the BTF
attribute.

The actual semantics of the pinning is similar to the iproute2 "PIN_GLOBAL"
setting, and the eventual goal is to move the iproute2 implementation to be
based on libbpf and the functions introduced in this series.

Changelog:

v2:
  - Drop patch that adds mounting of bpffs
  - Only support a single value of the pinning attribute
  - Add patch to fixup error handling in reuse_fd()
  - Implement the full automatic pinning and map reuse logic on load

---

Toke Høiland-Jørgensen (4):
      libbpf: Fix error handling in bpf_map__reuse_fd()
      libbpf: Store map pin path and status in struct bpf_map
      libbpf: Support configurable pinning of maps from BTF annotations
      libbpf: Add option to auto-pin maps when opening BPF object


 tools/lib/bpf/bpf_helpers.h |    6 +
 tools/lib/bpf/libbpf.c      |  379 +++++++++++++++++++++++++++++++++++++------
 tools/lib/bpf/libbpf.h      |   33 ++++
 tools/lib/bpf/libbpf.map    |    6 +
 4 files changed, 366 insertions(+), 58 deletions(-)

[PATCH bpf-next v2 1/4] libbpf: Fix error handling in bpf_map__reuse_fd()

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

bpf_map__reuse_fd() was calling close() in the error path before returning
an error value based on errno. However, close can change errno, so that can
lead to potentially misleading error messages. Instead, explicitly store
errno in the err variable before each goto.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index cccfd9355134..a2a7d074ac48 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1918,16 +1918,22 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
 		return -errno;
 
 	new_fd = open("/", O_RDONLY | O_CLOEXEC);
-	if (new_fd < 0)
+	if (new_fd < 0) {
+		err = -errno;
 		goto err_free_new_name;
+	}
 
 	new_fd = dup3(fd, new_fd, O_CLOEXEC);
-	if (new_fd < 0)
+	if (new_fd < 0) {
+		err = -errno;
 		goto err_close_new_fd;
+	}
 
 	err = zclose(map->fd);
-	if (err)
+	if (err) {
+		err = -errno;
 		goto err_close_new_fd;
+	}
 	free(map->name);
 
 	map->fd = new_fd;
@@ -1946,7 +1952,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
 	close(new_fd);
 err_free_new_name:
 	free(new_name);
-	return -errno;
+	return err;
 }
 
 int bpf_map__resize(struct bpf_map *map, __u32 max_entries)

Re: [PATCH bpf-next v2 1/4] libbpf: Fix error handling in bpf_map__reuse_fd()

[Andrii Nakryiko]

On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> bpf_map__reuse_fd() was calling close() in the error path before returning
> an error value based on errno. However, close can change errno, so that can
> lead to potentially misleading error messages. Instead, explicitly store
> errno in the err variable before each goto.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Thanks!

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/lib/bpf/libbpf.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index cccfd9355134..a2a7d074ac48 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1918,16 +1918,22 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
>                 return -errno;
>
>         new_fd = open("/", O_RDONLY | O_CLOEXEC);
> -       if (new_fd < 0)
> +       if (new_fd < 0) {
> +               err = -errno;
>                 goto err_free_new_name;
> +       }
>
>         new_fd = dup3(fd, new_fd, O_CLOEXEC);
> -       if (new_fd < 0)
> +       if (new_fd < 0) {
> +               err = -errno;
>                 goto err_close_new_fd;
> +       }
>
>         err = zclose(map->fd);
> -       if (err)
> +       if (err) {
> +               err = -errno;
>                 goto err_close_new_fd;
> +       }
>         free(map->name);
>
>         map->fd = new_fd;
> @@ -1946,7 +1952,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
>         close(new_fd);
>  err_free_new_name:
>         free(new_name);
> -       return -errno;
> +       return err;
>  }
>
>  int bpf_map__resize(struct bpf_map *map, __u32 max_entries)
>

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

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

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

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 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 a2a7d074ac48..848e6710b8e6 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 {
@@ -4013,47 +4015,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_warning("invalid map pointer\n");
 		return -EINVAL;
 	}
 
-	if (bpf_obj_pin(map->fd, path)) {
-		cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
-		pr_warning("failed to pin map: %s\n", cp);
-		return -errno;
+	if (map->pinned) {
+		pr_warning("map already pinned\n");
+		return -EEXIST;
+	}
+
+	if (path && map->pin_path && strcmp(path, map->pin_path)) {
+		pr_warning("map already has pin path '%s' different from '%s'\n",
+			   map->pin_path, path);
+		return -EINVAL;
+	}
+
+	if (!map->pin_path && !path) {
+		pr_warning("missing pin path\n");
+		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;
+		}
+	}
+
+	err = check_path(map->pin_path);
+	if (err)
+		return err;
+
+	if (bpf_obj_pin(map->fd, map->pin_path)) {
+		err = -errno;
+		goto out_err;
+	}
+
+	map->pinned = true;
+	pr_debug("pinned map '%s'\n", map->pin_path);
 
 	return 0;
+
+out_err:
+	cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg));
+	pr_warning("failed to pin map: %s\n", cp);
+	return err;
 }
 
 int bpf_map__unpin(struct bpf_map *map, const char *path)
 {
 	int err;
 
-	err = check_path(path);
-	if (err)
-		return err;
-
 	if (map == NULL) {
 		pr_warning("invalid map pointer\n");
 		return -EINVAL;
 	}
 
-	err = unlink(path);
+	if (!map->pin_path) {
+		pr_warning("map has no pin_path set\n");
+		return -ENOENT;
+	}
+
+	if (path && strcmp(path, map->pin_path)) {
+		pr_warning("unpin path '%s' differs from map pin path '%s'\n",
+			   path, map->pin_path);
+		return -EINVAL;
+	}
+
+	err = check_path(map->pin_path);
+	if (err)
+		return err;
+
+	err = unlink(map->pin_path);
 	if (err != 0)
 		return -errno;
-	pr_debug("unpinned map '%s'\n", path);
+
+	map->pinned = false;
+	pr_debug("unpinned map '%s'\n", map->pin_path);
 
 	return 0;
 }
 
+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);
+
+	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;
@@ -4094,17 +4167,10 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 
 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);
 	}
 
 	return err;
@@ -4254,6 +4320,7 @@ void bpf_object__close(struct bpf_object *obj)
 
 	for (i = 0; i < obj->nr_maps; i++) {
 		zfree(&obj->maps[i].name);
+		zfree(&obj->maps[i].pin_path);
 		if (obj->maps[i].clear_priv)
 			obj->maps[i].clear_priv(&obj->maps[i],
 						obj->maps[i].priv);
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 53ce212764e0..4e6733df5bb4 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -375,6 +375,9 @@ LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries);
 LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map);
 LIBBPF_API bool bpf_map__is_internal(const struct bpf_map *map);
 LIBBPF_API void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex);
+LIBBPF_API int bpf_map__set_pin_path(struct bpf_map *map, const char *path);
+LIBBPF_API const char *bpf_map__get_pin_path(struct bpf_map *map);
+LIBBPF_API bool bpf_map__is_pinned(struct bpf_map *map);
 LIBBPF_API int bpf_map__pin(struct bpf_map *map, const char *path);
 LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 4d241fd92dd4..ef9ae1943ec7 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -195,4 +195,7 @@ LIBBPF_0.0.6 {
 	global:
 		bpf_object__open_file;
 		bpf_object__open_mem;
+		bpf_map__get_pin_path;
+		bpf_map__set_pin_path;
+		bpf_map__is_pinned;
 } LIBBPF_0.0.5;

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

[Andrii Nakryiko]

On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> 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).
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Looks good!

Acked-by: Andrii Nakryiko <andriin@fb.com>

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

[PATCH bpf-next v2 3/4] libbpf: Support configurable pinning of maps from BTF annotations

[Toke Høiland-Jørgensen]

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. We introduce a version new
bpf_object__map_pin_opts() function to pin maps based on this setting, as
well as a getter and setter function for the pin information that callers
can use after map 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.

The pinning options supports a 'pin_all' setting, which corresponds to the
old bpf_object__map_pin() function with an explicit path. In addition, the
function now defaults to just skipping over maps that are already
pinned (since the previous commit started recording this in struct
bpf_map). This behaviour can be turned off with the 'no_skip_pinned' option.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/bpf_helpers.h |    6 ++
 tools/lib/bpf/libbpf.c      |  134 ++++++++++++++++++++++++++++++++++---------
 tools/lib/bpf/libbpf.h      |   26 ++++++++
 tools/lib/bpf/libbpf.map    |    3 +
 4 files changed, 142 insertions(+), 27 deletions(-)

diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 2203595f38c3..0c7d28292898 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -38,4 +38,10 @@ struct bpf_map_def {
 	unsigned int map_flags;
 };
 
+enum libbpf_pin_type {
+	LIBBPF_PIN_NONE,
+	/* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
+	LIBBPF_PIN_BY_NAME,
+};
+
 #endif
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 848e6710b8e6..179c9911458d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -226,6 +226,7 @@ struct bpf_map {
 	void *priv;
 	bpf_map_clear_priv_t clear_priv;
 	enum libbpf_map_type libbpf_type;
+	enum libbpf_pin_type pinning;
 	char *pin_path;
 	bool pinned;
 };
@@ -1271,6 +1272,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
 			}
 			map->def.value_size = sz;
 			map->btf_value_type_id = t->type;
+		} else if (strcmp(name, "pinning") == 0) {
+			__u32 val;
+
+			if (!get_map_field_int(map_name, obj->btf, def, m,
+					       &val))
+				return -EINVAL;
+			pr_debug("map '%s': found pinning = %u.\n",
+				 map_name, val);
+
+			if (val != LIBBPF_PIN_NONE &&
+			    val != LIBBPF_PIN_BY_NAME) {
+				pr_warning("map '%s': invalid pinning value %u.\n",
+					   map_name, val);
+				return -EINVAL;
+			}
+			map->pinning = val;
 		} else {
 			if (strict) {
 				pr_warning("map '%s': unknown field '%s'.\n",
@@ -1340,6 +1357,27 @@ static int bpf_object__init_user_btf_maps(struct bpf_object *obj, bool strict)
 	return 0;
 }
 
+static int build_pin_path(char *buf, size_t buf_len,
+			  struct bpf_map *map,
+			  const char *path,
+			  bool pin_all)
+{
+	int len;
+
+	if (map->pinning != LIBBPF_PIN_BY_NAME && !pin_all)
+		return 0;
+
+	if (!path)
+		path = "/sys/fs/bpf";
+
+	len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map));
+	if (len < 0)
+		return -EINVAL;
+	else if (len >= buf_len)
+		return -ENAMETOOLONG;
+	return len;
+}
+
 static int bpf_object__init_maps(struct bpf_object *obj, bool relaxed_maps)
 {
 	bool strict = !relaxed_maps;
@@ -4127,10 +4165,13 @@ bool bpf_map__is_pinned(struct bpf_map *map)
 	return map->pinned;
 }
 
-int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
+int bpf_object__pin_maps_opts(struct bpf_object *obj,
+			      struct bpf_object_pin_opts *opts)
 {
+	bool pin_all, skip_pinned;
+	const char *pin_path;
 	struct bpf_map *map;
-	int err;
+	int err, len;
 
 	if (!obj)
 		return -ENOENT;
@@ -4140,25 +4181,34 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 		return -ENOENT;
 	}
 
-	err = make_dir(path);
-	if (err)
-		return err;
+	if (!OPTS_VALID(opts, bpf_object_pin_opts))
+		return -EINVAL;
+
+	pin_path = OPTS_GET(opts, pin_path, NULL);
+	pin_all = OPTS_GET(opts, pin_all, false);
+	skip_pinned = !OPTS_GET(opts, no_skip_pinned, false);
 
 	bpf_object__for_each_map(map, obj) {
+		char *path = NULL;
 		char buf[PATH_MAX];
-		int len;
 
-		len = snprintf(buf, PATH_MAX, "%s/%s", path,
-			       bpf_map__name(map));
-		if (len < 0) {
-			err = -EINVAL;
-			goto err_unpin_maps;
-		} else if (len >= PATH_MAX) {
-			err = -ENAMETOOLONG;
-			goto err_unpin_maps;
+		if (map->pinned && skip_pinned)
+			continue;
+
+		if (!map->pin_path) {
+
+			len = build_pin_path(buf, sizeof(buf), map,
+					     pin_path, pin_all);
+			if (len == 0) {
+				continue;
+			} else if (len < 0) {
+				err = len;
+				goto err_unpin_maps;
+			}
+			path = buf;
 		}
 
-		err = bpf_map__pin(map, buf);
+		err = bpf_map__pin(map, path);
 		if (err)
 			goto err_unpin_maps;
 	}
@@ -4166,16 +4216,30 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 	return 0;
 
 err_unpin_maps:
-	while ((map = bpf_map__prev(map, obj))) {
-		if (!map->pin_path)
-			continue;
+	if (!skip_pinned) {
+		/* Only undo pinning if we are not configured to skip
+		 * already-pinned maps; otherwise we could undo something we
+		 * didn't do in the first place.
+		 */
+		while ((map = bpf_map__prev(map, obj))) {
+			if (!map->pinned)
+				continue;
 
-		bpf_map__unpin(map, NULL);
+			bpf_map__unpin(map, NULL);
+		}
 	}
 
 	return err;
 }
 
+int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
+{
+	LIBBPF_OPTS(bpf_object_pin_opts, opts,
+		    .pin_path = path,
+		    .pin_all = (path != NULL));
+	return bpf_object__pin_maps_opts(obj, &opts);
+}
+
 int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
 {
 	struct bpf_map *map;
@@ -4185,18 +4249,24 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
 		return -ENOENT;
 
 	bpf_object__for_each_map(map, obj) {
+		char *pin_path = NULL;
 		char buf[PATH_MAX];
 		int len;
 
-		len = snprintf(buf, PATH_MAX, "%s/%s", path,
-			       bpf_map__name(map));
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (path) {
+			len = build_pin_path(buf, sizeof(buf), map, path, true);
+			if (len == 0)
+				continue;
+			else if (len < 0)
+				return len;
 
-		err = bpf_map__unpin(map, buf);
-		if (err)
+			pin_path = buf;
+		} else if (!map->pin_path) {
+			continue;
+		}
+
+		err = bpf_map__unpin(map, pin_path);
+		if (err && err != -ENOENT)
 			return err;
 	}
 
@@ -4854,6 +4924,16 @@ void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex)
 	map->map_ifindex = ifindex;
 }
 
+void bpf_map__set_pinning(struct bpf_map *map, enum libbpf_pin_type pinning)
+{
+	map->pinning = pinning;
+}
+
+enum libbpf_pin_type bpf_map__get_pinning(struct bpf_map *map)
+{
+	return map->pinning;
+}
+
 int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd)
 {
 	if (!bpf_map_type__is_map_in_map(map->def.type)) {
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 4e6733df5bb4..26a4ed3856e7 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -119,9 +119,33 @@ int bpf_object__section_size(const struct bpf_object *obj, const char *name,
 			     __u32 *size);
 int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
 				__u32 *off);
+
+enum libbpf_pin_type {
+	LIBBPF_PIN_NONE,
+	/* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
+	LIBBPF_PIN_BY_NAME,
+};
+
+struct bpf_object_pin_opts {
+	/* size of this struct, for forward/backward compatiblity */
+	size_t sz;
+
+	/* Paths to pin maps. Defaults to /sys/fs/bpf */
+	const char *pin_path;
+
+	/* If set, pin all maps regardless of map definition 'pinning' value */
+	bool pin_all;
+
+	/* If set, don't skip already pinned maps */
+	bool no_skip_pinned;
+};
+#define bpf_object_pin_opts__last_field no_skip_pinned
+
 LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);
 LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
 				      const char *path);
+LIBBPF_API int bpf_object__pin_maps_opts(struct bpf_object *obj,
+					 struct bpf_object_pin_opts *opts);
 LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
 					const char *path);
 LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
@@ -380,6 +404,8 @@ LIBBPF_API const char *bpf_map__get_pin_path(struct bpf_map *map);
 LIBBPF_API bool bpf_map__is_pinned(struct bpf_map *map);
 LIBBPF_API int bpf_map__pin(struct bpf_map *map, const char *path);
 LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
+LIBBPF_API enum libbpf_pin_type bpf_map__get_pinning(struct bpf_map *map);
+LIBBPF_API void bpf_map__set_pinning(struct bpf_map *map, enum libbpf_pin_type);
 
 LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd);
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index ef9ae1943ec7..94e0dbe1658b 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -198,4 +198,7 @@ LIBBPF_0.0.6 {
 		bpf_map__get_pin_path;
 		bpf_map__set_pin_path;
 		bpf_map__is_pinned;
+		bpf_map__get_pinning;
+		bpf_map__set_pinning;
+		bpf_object__pin_maps_opts;
 } LIBBPF_0.0.5;

Re: [PATCH bpf-next v2 3/4] libbpf: Support configurable pinning of maps from BTF annotations

[Toke Høiland-Jørgensen]

> +int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
> +{
> +	LIBBPF_OPTS(bpf_object_pin_opts, opts,
> +		    .pin_path = path,
> +		    .pin_all = (path != NULL));
> +	return bpf_object__pin_maps_opts(obj, &opts);
> +}

Hmm, seems I forgot to pull before sending; this should be
LIBBPF_DECLARE_OPTS now. Will fix in the next version, but I'll give
y'all a chance to comment on this version first :)

-Toke

Re: [PATCH bpf-next v2 3/4] libbpf: Support configurable pinning of maps from BTF annotations

[Andrii Nakryiko]

On Thu, Oct 24, 2019 at 6:11 AM 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. We introduce a version new
> bpf_object__map_pin_opts() function to pin maps based on this setting, as
> well as a getter and setter function for the pin information that callers
> can use after map 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.
>
> The pinning options supports a 'pin_all' setting, which corresponds to the
> old bpf_object__map_pin() function with an explicit path. In addition, the
> function now defaults to just skipping over maps that are already
> pinned (since the previous commit started recording this in struct
> bpf_map). This behaviour can be turned off with the 'no_skip_pinned' option.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

I think you are overcomplicating this... Here's how I think we can
satisfy both simplicity goals, as well as good usability:

1. add `const char *pin_root_path` to bpf_object_open_opts. This
pinning path override doesn't need to leave in some separate set of
options, it's BPF object's parameter, so let's put it into open
settings.

2. If BTF-defined map definition has pinning set to PIN_BY_NAME, that
means bpf_object__load should do auto-pinning. If not, no
auto-pinning, only if manually requested by explicit bpf_map__pin.
Further, if someone wants to auto-pin map to a custom location, do
bpf_map__set_pin_path() before bpf_object__load(), and load should
auto-pin it as well.

3. bpf_map__get_pinning/bpf_map__set_pinning are unnecessary, at least
for now. Let's not add unnecessary APIs.

4. pin_all/skip_pinned seems unnecessary. What scenarios are you
solving with them? Given #1 and #4, just drop
bpf_object__pin_maps_opts().

The way I see it, libbpf should behave sanely for declarative use
case, but allow custom tuning programmatically. If map is set to
PIN_BY_NAME in map definition - we derive pin_path (potentially taking
into account custom pin root path from open opts) and auto-pin on load
(unless application set pin_path manually). In a weird case, where map
is declaratively defined as auto-pinnable, but application for
whatever reason decides not to do it - it can unset pin_path with
bpf_map__set_pin_path(NULL).

Full control, but simple and intuitive default behavior? Does it make sense?


>  tools/lib/bpf/bpf_helpers.h |    6 ++
>  tools/lib/bpf/libbpf.c      |  134 ++++++++++++++++++++++++++++++++++---------
>  tools/lib/bpf/libbpf.h      |   26 ++++++++
>  tools/lib/bpf/libbpf.map    |    3 +
>  4 files changed, 142 insertions(+), 27 deletions(-)
>

Re: [PATCH bpf-next v2 3/4] libbpf: Support configurable pinning of maps from BTF annotations

[Jesper Dangaard Brouer]

On Thu, 24 Oct 2019 15:11:40 +0200
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. We introduce a version new
> bpf_object__map_pin_opts() function to pin maps based on this setting, as
> well as a getter and setter function for the pin information that callers
> can use after map 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.
> 
> The pinning options supports a 'pin_all' setting, which corresponds to the
> old bpf_object__map_pin() function with an explicit path. In addition, the
> function now defaults to just skipping over maps that are already
> pinned (since the previous commit started recording this in struct
> bpf_map). This behaviour can be turned off with the 'no_skip_pinned' option.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/bpf_helpers.h |    6 ++
>  tools/lib/bpf/libbpf.c      |  134 ++++++++++++++++++++++++++++++++++---------
>  tools/lib/bpf/libbpf.h      |   26 ++++++++
>  tools/lib/bpf/libbpf.map    |    3 +
>  4 files changed, 142 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 2203595f38c3..0c7d28292898 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -38,4 +38,10 @@ struct bpf_map_def {
>  	unsigned int map_flags;
>  };
>  
> +enum libbpf_pin_type {
> +	LIBBPF_PIN_NONE,
> +	/* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
> +	LIBBPF_PIN_BY_NAME,
> +};
> +
>  #endif
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 848e6710b8e6..179c9911458d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -226,6 +226,7 @@ struct bpf_map {
>  	void *priv;
>  	bpf_map_clear_priv_t clear_priv;
>  	enum libbpf_map_type libbpf_type;
> +	enum libbpf_pin_type pinning;
>  	char *pin_path;
>  	bool pinned;
>  };
> @@ -1271,6 +1272,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
>  			}
>  			map->def.value_size = sz;
>  			map->btf_value_type_id = t->type;
> +		} else if (strcmp(name, "pinning") == 0) {
> +			__u32 val;
> +
> +			if (!get_map_field_int(map_name, obj->btf, def, m,
> +					       &val))
> +				return -EINVAL;
> +			pr_debug("map '%s': found pinning = %u.\n",
> +				 map_name, val);
> +
> +			if (val != LIBBPF_PIN_NONE &&
> +			    val != LIBBPF_PIN_BY_NAME) {
> +				pr_warning("map '%s': invalid pinning value %u.\n",
> +					   map_name, val);
> +				return -EINVAL;
> +			}
> +			map->pinning = val;
>  		} else {
>  			if (strict) {
>  				pr_warning("map '%s': unknown field '%s'.\n",
[...]

How does this prepare for being compatible with iproute2 pinning?

iproute2 have these defines (in include/bpf_elf.h):

 /* Object pinning settings */
 #define PIN_NONE                0
 #define PIN_OBJECT_NS           1
 #define PIN_GLOBAL_NS           2

I do know above strcmp(name, "pinning") look at BTF info 'name' and not
directly at the ELF struct for maps.  I don't know enough about BTF
(yet), but won't BTF automatically create a "pinning" info 'name' ???
(with above defines as content/values)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

From above enum:
 LIBBPF_PIN_BY_NAME = 1 

iproute2 ELF map struct:

/* ELF map definition */
struct bpf_elf_map {
        __u32 type;
        __u32 size_key;
        __u32 size_value;
        __u32 max_elem;
        __u32 flags;
        __u32 id;
        __u32 pinning;
        __u32 inner_id;
        __u32 inner_idx;
};

Re: [PATCH bpf-next v2 3/4] libbpf: Support configurable pinning of maps from BTF annotations

[Andrii Nakryiko]

On Fri, Oct 25, 2019 at 5:32 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Thu, 24 Oct 2019 15:11:40 +0200
> 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. We introduce a version new
> > bpf_object__map_pin_opts() function to pin maps based on this setting, as
> > well as a getter and setter function for the pin information that callers
> > can use after map 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.
> >
> > The pinning options supports a 'pin_all' setting, which corresponds to the
> > old bpf_object__map_pin() function with an explicit path. In addition, the
> > function now defaults to just skipping over maps that are already
> > pinned (since the previous commit started recording this in struct
> > bpf_map). This behaviour can be turned off with the 'no_skip_pinned' option.
> >
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> >  tools/lib/bpf/bpf_helpers.h |    6 ++
> >  tools/lib/bpf/libbpf.c      |  134 ++++++++++++++++++++++++++++++++++---------
> >  tools/lib/bpf/libbpf.h      |   26 ++++++++
> >  tools/lib/bpf/libbpf.map    |    3 +
> >  4 files changed, 142 insertions(+), 27 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index 2203595f38c3..0c7d28292898 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -38,4 +38,10 @@ struct bpf_map_def {
> >       unsigned int map_flags;
> >  };
> >
> > +enum libbpf_pin_type {
> > +     LIBBPF_PIN_NONE,
> > +     /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
> > +     LIBBPF_PIN_BY_NAME,
> > +};
> > +
> >  #endif
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 848e6710b8e6..179c9911458d 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -226,6 +226,7 @@ struct bpf_map {
> >       void *priv;
> >       bpf_map_clear_priv_t clear_priv;
> >       enum libbpf_map_type libbpf_type;
> > +     enum libbpf_pin_type pinning;
> >       char *pin_path;
> >       bool pinned;
> >  };
> > @@ -1271,6 +1272,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> >                       }
> >                       map->def.value_size = sz;
> >                       map->btf_value_type_id = t->type;
> > +             } else if (strcmp(name, "pinning") == 0) {
> > +                     __u32 val;
> > +
> > +                     if (!get_map_field_int(map_name, obj->btf, def, m,
> > +                                            &val))
> > +                             return -EINVAL;
> > +                     pr_debug("map '%s': found pinning = %u.\n",
> > +                              map_name, val);
> > +
> > +                     if (val != LIBBPF_PIN_NONE &&
> > +                         val != LIBBPF_PIN_BY_NAME) {
> > +                             pr_warning("map '%s': invalid pinning value %u.\n",
> > +                                        map_name, val);
> > +                             return -EINVAL;
> > +                     }
> > +                     map->pinning = val;
> >               } else {
> >                       if (strict) {
> >                               pr_warning("map '%s': unknown field '%s'.\n",
> [...]
>
> How does this prepare for being compatible with iproute2 pinning?
>
> iproute2 have these defines (in include/bpf_elf.h):
>
>  /* Object pinning settings */
>  #define PIN_NONE                0
>  #define PIN_OBJECT_NS           1
>  #define PIN_GLOBAL_NS           2
>
> I do know above strcmp(name, "pinning") look at BTF info 'name' and not
> directly at the ELF struct for maps.  I don't know enough about BTF
> (yet), but won't BTF automatically create a "pinning" info 'name' ???
> (with above defines as content/values)

We are not supporting iproute2's BTF map definitions as is, we are
trying to support all the functionality needed to support iproute2's
ways of doing things, but it will require iproute2 to do some gluing,
of course. We don't intend to support any conceivable legacy format
out there in libbpf, rather make libbpf powerful, flexible, and
expressive enough to support those use case, but with the help of
tools like iprout2 to do "translations" necessary.

For "modern" iprout2 BPF programs that are using BTF-defined maps and
stuff - yes, that will work as is without iproute2 having to do much.

>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>
> From above enum:
>  LIBBPF_PIN_BY_NAME = 1
>
> iproute2 ELF map struct:
>
> /* ELF map definition */
> struct bpf_elf_map {
>         __u32 type;
>         __u32 size_key;
>         __u32 size_value;
>         __u32 max_elem;
>         __u32 flags;
>         __u32 id;
>         __u32 pinning;
>         __u32 inner_id;
>         __u32 inner_idx;
> };
>

[PATCH bpf-next v2 4/4] libbpf: Add option to auto-pin maps when opening BPF object

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.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@redhat.com>
---
 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)
+{
+	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);
+		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) {
+		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);
+		}
+
 		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)
+{
+	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)
+{
+	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);
+			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 *

Re: [PATCH bpf-next v2 4/4] libbpf: Add option to auto-pin maps when opening BPF object

[Andrii Nakryiko]

On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.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@redhat.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 *
>

Re: [PATCH bpf-next v2 4/4] libbpf: Add option to auto-pin maps when opening BPF object

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.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@redhat.com>
>> ---
>
> How have you tested this? From reading the code, all the maps will be
> pinned irregardless of their .pinning setting?

No, build_pin_path() checks map->pinning :)

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

Can do.

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

But I like 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? ;)

Heh, I guess I can cut down the number of options to the number of tests :P

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

We'll need at least two passes: set pin_path on open, and check reuse /
create / pin on load. Don't have any objections to consolidating the
other passes into create_maps; will fix, along with your comments below.

-Toke

Re: [PATCH bpf-next v2 4/4] libbpf: Add option to auto-pin maps when opening BPF object

[Andrii Nakryiko]

On Sun, Oct 27, 2019 at 5:04 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@redhat.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@redhat.com>
> >> ---
> >
> > How have you tested this? From reading the code, all the maps will be
> > pinned irregardless of their .pinning setting?
>
> No, build_pin_path() checks map->pinning :)

subtle... build_pin_path() definitely doesn't imply that it's a "maybe
build pin path?", but see below for pin_path setting.

>
> > Please add proper tests to test_progs, testing various modes and
> > overrides.
>
> Can do.
>
> > 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.
>
> But I like 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? ;)
>
> Heh, I guess I can cut down the number of options to the number of tests :P
>
> > 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.
>
> We'll need at least two passes: set pin_path on open, and check reuse /
> create / pin on load. Don't have any objections to consolidating the
> other passes into create_maps; will fix, along with your comments below.

for BTF-defined maps, can't we just set a pin_path right when we are
reading map definition? With that, we won't even need to store
.pinning field. Would that work?

>
> -Toke

Re: [PATCH bpf-next v2 4/4] libbpf: Add option to auto-pin maps when opening BPF object

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Sun, Oct 27, 2019 at 5:04 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> From: Toke Høiland-Jørgensen <toke@redhat.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@redhat.com>
>> >> ---
>> >
>> > How have you tested this? From reading the code, all the maps will be
>> > pinned irregardless of their .pinning setting?
>>
>> No, build_pin_path() checks map->pinning :)
>
> subtle... build_pin_path() definitely doesn't imply that it's a "maybe
> build pin path?", but see below for pin_path setting.
>
>>
>> > Please add proper tests to test_progs, testing various modes and
>> > overrides.
>>
>> Can do.
>>
>> > 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.
>>
>> But I like 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? ;)
>>
>> Heh, I guess I can cut down the number of options to the number of tests :P
>>
>> > 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.
>>
>> We'll need at least two passes: set pin_path on open, and check reuse /
>> create / pin on load. Don't have any objections to consolidating the
>> other passes into create_maps; will fix, along with your comments below.
>
> for BTF-defined maps, can't we just set a pin_path right when we are
> reading map definition? With that, we won't even need to store
> .pinning field. Would that work?

We could, and I did actually try that. However, I think it is more
readable to have it be a separate step: init_user_btf_maps() parses the
map def, and after that is done we loop over things to build the pin
paths.

I'll send a v3 in a bit, you can see for yourself :)

-Toke