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

[PATCH bpf-next v3 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 open option which can set the pinning path;
if no path is set, /sys/fs/bpf is used as the default. Callers can customise the
pinning between open and load by setting the pin path per map, and still get the
automatic reuse feature.

The 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:

v3:
  - Drop bpf_object__pin_maps_opts() and just use an open option to customise
    the pin path; also don't touch bpf_object__{un,}pin_maps()
  - Integrate pinning and reuse into bpf_object__create_maps() instead of having
    multiple loops though the map structure
  - Make errors in map reuse and pinning fatal to the load procedure
  - Add selftest to exercise pinning feature
  - Rebase series to latest bpf-next

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: Add auto-pinning of maps when loading BPF objects
      selftests: Add tests for automatic map pinning


 tools/lib/bpf/bpf_helpers.h                      |    6 
 tools/lib/bpf/libbpf.c                           |  271 +++++++++++++++++++---
 tools/lib/bpf/libbpf.h                           |   14 +
 tools/lib/bpf/libbpf.map                         |    3 
 tools/testing/selftests/bpf/prog_tests/pinning.c |   91 +++++++
 tools/testing/selftests/bpf/progs/test_pinning.c |   29 ++
 6 files changed, 381 insertions(+), 33 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/pinning.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_pinning.c

[PATCH bpf-next v3 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.

Acked-by: Andrii Nakryiko <andriin@fb.com>
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 d71631a01926..ce5ef3ddd263 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1916,16 +1916,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;
@@ -1944,7 +1950,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 v3 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).

Acked-by: Andrii Nakryiko <andriin@fb.com>
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 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");
+		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);
+		return -EINVAL;
+	}
+
+	if (!map->pin_path && !path) {
+		pr_warn("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_warn("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_warn("invalid map pointer\n");
 		return -EINVAL;
 	}
 
-	err = unlink(path);
+	if (!map->pin_path) {
+		pr_warn("map has no pin_path set\n");
+		return -ENOENT;
+	}
+
+	if (path && strcmp(path, map->pin_path)) {
+		pr_warn("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;
@@ -4106,17 +4179,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;
@@ -4266,6 +4332,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 c63e2ff84abc..a514729c43f5 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -385,6 +385,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 d1473ea4d7a5..c24d4c01591d 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -197,4 +197,7 @@ LIBBPF_0.0.6 {
 		bpf_object__open_mem;
 		bpf_program__get_expected_attach_type;
 		bpf_program__get_type;
+		bpf_map__get_pin_path;
+		bpf_map__set_pin_path;
+		bpf_map__is_pinned;
 } LIBBPF_0.0.5;

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

[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, 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 <toke@redhat.com>
---
 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(-)

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 eb1c5e6ad4a3..96d0eca6fa74 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;
 };
@@ -1270,6 +1271,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_warn("map '%s': invalid pinning value %u.\n",
+					map_name, val);
+				return -EINVAL;
+			}
+			map->pinning = val;
 		} else {
 			if (strict) {
 				pr_warn("map '%s': unknown field '%s'.\n",
@@ -1339,7 +1356,36 @@ static int bpf_object__init_user_btf_maps(struct bpf_object *obj, bool strict)
 	return 0;
 }
 
-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;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
+		if (len < 0)
+			return -EINVAL;
+		else if (len >= PATH_MAX)
+			return -ENAMETOOLONG;
+
+		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,
+				 const char *auto_pin_path)
 {
 	bool strict = !relaxed_maps;
 	int err;
@@ -1356,6 +1402,10 @@ static int bpf_object__init_maps(struct bpf_object *obj, bool relaxed_maps)
 	if (err)
 		return err;
 
+	err = bpf_object__build_map_pin_paths(obj, auto_pin_path);
+	if (err)
+		return err;
+
 	if (obj->nr_maps) {
 		qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]),
 		      compare_bpf_map);
@@ -1537,7 +1587,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,
+				   const char *auto_pin_path)
 {
 	Elf *elf = obj->efile.elf;
 	GElf_Ehdr *ep = &obj->efile.ehdr;
@@ -1672,7 +1723,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_path);
 	if (!err)
 		err = bpf_object__sanitize_and_load_btf(obj);
 	if (!err)
@@ -2128,6 +2179,68 @@ bpf_object__probe_caps(struct bpf_object *obj)
 	return 0;
 }
 
+static bool map_is_reuse_compat(const struct bpf_map *map,
+				int map_fd)
+{
+	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);
+}
+
+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;
+	}
+
+	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;
+}
+
 static int
 bpf_object__populate_internal_map(struct bpf_object *obj, struct bpf_map *map)
 {
@@ -2170,6 +2283,15 @@ bpf_object__create_maps(struct bpf_object *obj)
 		char *cp, errmsg[STRERR_BUFSIZE];
 		int *pfd = &map->fd;
 
+		if (map->pin_path) {
+			err = bpf_object__reuse_map(map);
+			if (err) {
+				pr_warn("error reusing pinned map %s\n",
+					map->name);
+				return err;
+			}
+		}
+
 		if (map->fd >= 0) {
 			pr_debug("skip map create (preset) %s: fd=%d\n",
 				 map->name, map->fd);
@@ -2248,6 +2370,15 @@ bpf_object__create_maps(struct bpf_object *obj)
 			}
 		}
 
+		if (map->pin_path && !map->pinned) {
+			err = bpf_map__pin(map, NULL);
+			if (err) {
+				pr_warn("failed to auto-pin map name '%s' at '%s'\n",
+					map->name, map->pin_path);
+				return err;
+			}
+		}
+
 		pr_debug("created map %s: fd=%d\n", map->name, *pfd);
 	}
 
@@ -3619,6 +3750,7 @@ static struct bpf_object *
 __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 		   struct bpf_object_open_opts *opts)
 {
+	const char *auto_pin_path;
 	struct bpf_program *prog;
 	struct bpf_object *obj;
 	const char *obj_name;
@@ -3653,11 +3785,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_path = OPTS_GET(opts, auto_pin_path, NULL);
 
 	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_path),
+		  err, out);
 	CHECK_ERR(bpf_object__collect_reloc(obj), err, out);
 	bpf_object__elf_finish(obj);
 
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index a514729c43f5..47e2f7dabf02 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -103,8 +103,10 @@ struct bpf_object_open_opts {
 	bool relaxed_maps;
 	/* process CO-RE relocations non-strictly, allowing them to fail */
 	bool relaxed_core_relocs;
+	/* path to auto-pin maps with 'pinning' under (default /sys/fs/bpf) */
+	const char *auto_pin_path;
 };
-#define bpf_object_open_opts__last_field relaxed_core_relocs
+#define bpf_object_open_opts__last_field auto_pin_path
 
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
@@ -124,6 +126,13 @@ 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,
+};
+
 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);

[PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Toke Høiland-Jørgensen]

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

This adds a new BPF selftest to exercise the new automatic map pinning
code.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/testing/selftests/bpf/prog_tests/pinning.c |   91 ++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_pinning.c |   29 +++++++
 2 files changed, 120 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/pinning.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_pinning.c

diff --git a/tools/testing/selftests/bpf/prog_tests/pinning.c b/tools/testing/selftests/bpf/prog_tests/pinning.c
new file mode 100644
index 000000000000..d4a63de72f5a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <test_progs.h>
+
+__u32 get_map_id(struct bpf_object *obj, const char *name)
+{
+	__u32 map_info_len, duration, retval;
+	struct bpf_map_info map_info = {};
+	struct bpf_map *map;
+	int err;
+
+	map_info_len = sizeof(map_info);
+
+	map = bpf_object__find_map_by_name(obj, name);
+	if (!CHECK(!map, "find map", "NULL map")) {
+		err = bpf_obj_get_info_by_fd(bpf_map__fd(map),
+					     &map_info, &map_info_len);
+		CHECK(err, "get map info", "err %d errno %d", err, errno);
+		return map_info.id;
+	}
+	return 0;
+}
+
+void test_pinning(void)
+{
+	__u32 duration, retval, size, map_id, map_id2;
+	const char *custpinpath = "/sys/fs/bpf/custom/pinmap";
+	const char *nopinpath = "/sys/fs/bpf/nopinmap";
+	const char *custpath = "/sys/fs/bpf/custom";
+	const char *pinpath = "/sys/fs/bpf/pinmap";
+	const char *file = "./test_pinning.o";
+	struct stat statbuf = {};
+	struct bpf_object *obj;
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+		.auto_pin_path = custpath,
+	);
+
+	int err;
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK_FAIL(libbpf_get_error(obj)))
+		return;
+
+	err = bpf_object__load(obj);
+	CHECK(err, "default load", "err %d errno %d\n", err, errno);
+
+	/* check that pinmap was pinned */
+	err = stat(pinpath, &statbuf);
+	CHECK(err, "stat pinpath", "err %d errno %d\n", err, errno);
+
+        /* check that nopinmap was *not* pinned */
+	err = stat(nopinpath, &statbuf);
+	CHECK(errno != ENOENT, "stat nopinpath", "err %d errno %d\n", err, errno);
+
+        map_id = get_map_id(obj, "pinmap");
+	bpf_object__close(obj);
+
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK_FAIL(libbpf_get_error(obj)))
+		return;
+
+	err = bpf_object__load(obj);
+	CHECK(err, "default load", "err %d errno %d\n", err, errno);
+
+	/* check that same map ID was reused for second load */
+	map_id2 = get_map_id(obj, "pinmap");
+	CHECK(map_id != map_id2, "check reuse",
+	      "err %d errno %d id %d id2 %d\n", err, errno, map_id, map_id2);
+	unlink(pinpath);
+	bpf_object__close(obj);
+
+	err = mkdir(custpath, 0700);
+	CHECK(err, "mkdir custpath",  "err %d errno %d\n", err, errno);
+
+	obj = bpf_object__open_file(file, &opts);
+	if (CHECK_FAIL(libbpf_get_error(obj)))
+		return;
+
+	err = bpf_object__load(obj);
+	CHECK(err, "custom load", "err %d errno %d\n", err, errno);
+
+	/* check that pinmap was pinned at the custom path */
+	err = stat(custpinpath, &statbuf);
+	CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno);
+
+	unlink(custpinpath);
+	rmdir(custpath);
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
new file mode 100644
index 000000000000..ff2d7447777e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_pinning.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+int _version SEC("version") = 1;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u64);
+	__uint(pinning, LIBBPF_PIN_BY_NAME);
+} pinmap SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u64);
+} nopinmap SEC(".maps");
+
+SEC("xdp_prog")
+int _xdp_prog(struct xdp_md *xdp)
+{
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";

Re: [PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Jesper Dangaard Brouer]

On Sun, 27 Oct 2019 21:53:19 +0100
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
> new file mode 100644
> index 000000000000..ff2d7447777e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_pinning.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include "bpf_helpers.h"
> +
> +int _version SEC("version") = 1;
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, 1);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +	__uint(pinning, LIBBPF_PIN_BY_NAME);
> +} pinmap SEC(".maps");

So, this is the new BTF-defined maps syntax.

Please remind me, what version of LLVM do we need to compile this?

Or was there a dependency on pahole?


> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, 1);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +} nopinmap SEC(".maps");
> +
> +SEC("xdp_prog")
> +int _xdp_prog(struct xdp_md *xdp)
> +{
> +	return XDP_PASS;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> 



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

Re: [PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Toke Høiland-Jørgensen]

Jesper Dangaard Brouer <brouer@redhat.com> writes:

> On Sun, 27 Oct 2019 21:53:19 +0100
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>> diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
>> new file mode 100644
>> index 000000000000..ff2d7447777e
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_pinning.c
>> @@ -0,0 +1,29 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/bpf.h>
>> +#include "bpf_helpers.h"
>> +
>> +int _version SEC("version") = 1;
>> +
>> +struct {
>> +	__uint(type, BPF_MAP_TYPE_ARRAY);
>> +	__uint(max_entries, 1);
>> +	__type(key, __u32);
>> +	__type(value, __u64);
>> +	__uint(pinning, LIBBPF_PIN_BY_NAME);
>> +} pinmap SEC(".maps");
>
> So, this is the new BTF-defined maps syntax.
>
> Please remind me, what version of LLVM do we need to compile this?

No idea what the minimum version is. I'm running LLVM 9.0 :)

> Or was there a dependency on pahole?

Don't think so...

-Toke

Re: [PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Yonghong Song]

On 10/28/19 6:15 AM, Toke Høiland-Jørgensen wrote:
> Jesper Dangaard Brouer <brouer@redhat.com> writes:
> 
>> On Sun, 27 Oct 2019 21:53:19 +0100
>> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>>> diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
>>> new file mode 100644
>>> index 000000000000..ff2d7447777e
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/test_pinning.c
>>> @@ -0,0 +1,29 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +#include <linux/bpf.h>
>>> +#include "bpf_helpers.h"
>>> +
>>> +int _version SEC("version") = 1;
>>> +
>>> +struct {
>>> +	__uint(type, BPF_MAP_TYPE_ARRAY);
>>> +	__uint(max_entries, 1);
>>> +	__type(key, __u32);
>>> +	__type(value, __u64);
>>> +	__uint(pinning, LIBBPF_PIN_BY_NAME);
>>> +} pinmap SEC(".maps");
>>
>> So, this is the new BTF-defined maps syntax.
>>
>> Please remind me, what version of LLVM do we need to compile this?
> 
> No idea what the minimum version is. I'm running LLVM 9.0 :)

LLVM 9.0 starts to support .maps.
There is no dependency on pahole.

> 
>> Or was there a dependency on pahole?
> 
> Don't think so...
> 
> -Toke
> 

Re: [PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Jesper Dangaard Brouer]

On Mon, 28 Oct 2019 15:32:26 +0000
Yonghong Song <yhs@fb.com> wrote:

> On 10/28/19 6:15 AM, Toke Høiland-Jørgensen wrote:
> > Jesper Dangaard Brouer <brouer@redhat.com> writes:
> >   
> >> On Sun, 27 Oct 2019 21:53:19 +0100
> >> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>  
> >>> diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
> >>> new file mode 100644
> >>> index 000000000000..ff2d7447777e
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/bpf/progs/test_pinning.c
> >>> @@ -0,0 +1,29 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +#include <linux/bpf.h>
> >>> +#include "bpf_helpers.h"
> >>> +
> >>> +int _version SEC("version") = 1;
> >>> +
> >>> +struct {
> >>> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> >>> +	__uint(max_entries, 1);
> >>> +	__type(key, __u32);
> >>> +	__type(value, __u64);
> >>> +	__uint(pinning, LIBBPF_PIN_BY_NAME);
> >>> +} pinmap SEC(".maps");  
> >>
> >> So, this is the new BTF-defined maps syntax.
> >>
> >> Please remind me, what version of LLVM do we need to compile this?  
> > 
> > No idea what the minimum version is. I'm running LLVM 9.0 :)  
> 
> LLVM 9.0 starts to support .maps.
> There is no dependency on pahole.

LLVM 9.0.0 is still very new:
 - 19 September 2019: LLVM 9.0.0 is now available

For my XDP-tutorial[1], I cannot required people to have this new llvm
version.  But I would like to teach people about this new syntax (note,
I can upgrade libbpf version via git-submodule, and update bpf_helpers.h).

To Andrii, any recommendations on how I can do the transition? 

I'm thinking, it should be possible to define both ELF-object sections
SEC "maps" and ".maps" at the same time. But how does libbpf handle that?
(Who takes precedence?)


(Alternatively, I can detect the LLVM version, in the Makefile, and have
a #ifdef define in the code)
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

[1] https://github.com/xdp-project/xdp-tutorial

Re: [PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Alexei Starovoitov]

On Mon, Oct 28, 2019 at 9:13 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> LLVM 9.0.0 is still very new:
>  - 19 September 2019: LLVM 9.0.0 is now available
>
> For my XDP-tutorial[1], I cannot required people to have this new llvm
> version.

what's a concern?
It's a released version.
We recommend the latest kernel for new features. Same goes for compiler.

Re: [PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Andrii Nakryiko]

On Mon, Oct 28, 2019 at 9:13 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Mon, 28 Oct 2019 15:32:26 +0000
> Yonghong Song <yhs@fb.com> wrote:
>
> > On 10/28/19 6:15 AM, Toke Høiland-Jørgensen wrote:
> > > Jesper Dangaard Brouer <brouer@redhat.com> writes:
> > >
> > >> On Sun, 27 Oct 2019 21:53:19 +0100
> > >> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > >>
> > >>> diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
> > >>> new file mode 100644
> > >>> index 000000000000..ff2d7447777e
> > >>> --- /dev/null
> > >>> +++ b/tools/testing/selftests/bpf/progs/test_pinning.c
> > >>> @@ -0,0 +1,29 @@
> > >>> +// SPDX-License-Identifier: GPL-2.0
> > >>> +
> > >>> +#include <linux/bpf.h>
> > >>> +#include "bpf_helpers.h"
> > >>> +
> > >>> +int _version SEC("version") = 1;
> > >>> +
> > >>> +struct {
> > >>> + __uint(type, BPF_MAP_TYPE_ARRAY);
> > >>> + __uint(max_entries, 1);
> > >>> + __type(key, __u32);
> > >>> + __type(value, __u64);
> > >>> + __uint(pinning, LIBBPF_PIN_BY_NAME);
> > >>> +} pinmap SEC(".maps");
> > >>
> > >> So, this is the new BTF-defined maps syntax.
> > >>
> > >> Please remind me, what version of LLVM do we need to compile this?
> > >
> > > No idea what the minimum version is. I'm running LLVM 9.0 :)
> >
> > LLVM 9.0 starts to support .maps.
> > There is no dependency on pahole.
>
> LLVM 9.0.0 is still very new:
>  - 19 September 2019: LLVM 9.0.0 is now available
>
> For my XDP-tutorial[1], I cannot required people to have this new llvm
> version.  But I would like to teach people about this new syntax (note,
> I can upgrade libbpf version via git-submodule, and update bpf_helpers.h).
>
> To Andrii, any recommendations on how I can do the transition?
>
> I'm thinking, it should be possible to define both ELF-object sections
> SEC "maps" and ".maps" at the same time. But how does libbpf handle that?
> (Who takes precedence?)

Yes, libbpf will load both maps and .maps. There is no precedence,
they are treated equally and are just added to the list of maps. But
if there is .maps section without associated BTF, bpf_object__open
will fail (because BTF is mandatory at that point).

>
>
> (Alternatively, I can detect the LLVM version, in the Makefile, and have
> a #ifdef define in the code)
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>
> [1] https://github.com/xdp-project/xdp-tutorial
>

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

[Andrii Nakryiko]

On Sun, Oct 27, 2019 at 1:53 PM 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).
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
> 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 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

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

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

and here?

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


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

[...]

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

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

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


>         }
>
>         return err;
> @@ -4266,6 +4332,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 c63e2ff84abc..a514729c43f5 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -385,6 +385,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 d1473ea4d7a5..c24d4c01591d 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -197,4 +197,7 @@ LIBBPF_0.0.6 {
>                 bpf_object__open_mem;
>                 bpf_program__get_expected_attach_type;
>                 bpf_program__get_type;
> +               bpf_map__get_pin_path;
> +               bpf_map__set_pin_path;
> +               bpf_map__is_pinned;
>  } LIBBPF_0.0.5;
>

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

[Andrii Nakryiko]

On Sun, Oct 27, 2019 at 1:53 PM 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, 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 <toke@redhat.com>
> ---
>  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(-)
>

[...]

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


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

[...]

>         return 0;
>  }
>
> +static bool map_is_reuse_compat(const struct bpf_map *map,
> +                               int map_fd)

nit: this should fit on single line?

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

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


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

[...]

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



>  LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
>                                       const char *path);
>

Re: [PATCH bpf-next v3 4/4] selftests: Add tests for automatic map pinning

[Andrii Nakryiko]

On Sun, Oct 27, 2019 at 1:53 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This adds a new BPF selftest to exercise the new automatic map pinning
> code.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/pinning.c |   91 ++++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/test_pinning.c |   29 +++++++
>  2 files changed, 120 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/pinning.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_pinning.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/pinning.c b/tools/testing/selftests/bpf/prog_tests/pinning.c
> new file mode 100644
> index 000000000000..d4a63de72f5a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <test_progs.h>
> +
> +__u32 get_map_id(struct bpf_object *obj, const char *name)
> +{
> +       __u32 map_info_len, duration, retval;
> +       struct bpf_map_info map_info = {};
> +       struct bpf_map *map;
> +       int err;
> +
> +       map_info_len = sizeof(map_info);
> +
> +       map = bpf_object__find_map_by_name(obj, name);
> +       if (!CHECK(!map, "find map", "NULL map")) {

please follow the pattern of "if (CHECK()) { return or goto cleanup
}". There is literally zero cases where we have `if (!CHECK())` in
selftests.

> +               err = bpf_obj_get_info_by_fd(bpf_map__fd(map),
> +                                            &map_info, &map_info_len);
> +               CHECK(err, "get map info", "err %d errno %d", err, errno);
> +               return map_info.id;
> +       }
> +       return 0;
> +}
> +
> +void test_pinning(void)
> +{
> +       __u32 duration, retval, size, map_id, map_id2;
> +       const char *custpinpath = "/sys/fs/bpf/custom/pinmap";
> +       const char *nopinpath = "/sys/fs/bpf/nopinmap";
> +       const char *custpath = "/sys/fs/bpf/custom";
> +       const char *pinpath = "/sys/fs/bpf/pinmap";
> +       const char *file = "./test_pinning.o";
> +       struct stat statbuf = {};
> +       struct bpf_object *obj;
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> +               .auto_pin_path = custpath,
> +       );
> +
> +       int err;
> +       obj = bpf_object__open_file(file, NULL);
> +       if (CHECK_FAIL(libbpf_get_error(obj)))
> +               return;
> +
> +       err = bpf_object__load(obj);
> +       CHECK(err, "default load", "err %d errno %d\n", err, errno);

cleanup and exit, you don't have a valid set up to proceed with
testing. Same for almost every check below.

> +
> +       /* check that pinmap was pinned */
> +       err = stat(pinpath, &statbuf);
> +       CHECK(err, "stat pinpath", "err %d errno %d\n", err, errno);
> +
> +        /* check that nopinmap was *not* pinned */
> +       err = stat(nopinpath, &statbuf);
> +       CHECK(errno != ENOENT, "stat nopinpath", "err %d errno %d\n", err, errno);

if previous stat succeeded, errno might be from other syscall, you
have to check both

> +
> +        map_id = get_map_id(obj, "pinmap");

formatting? check that get_map_id succeeded?

> +       bpf_object__close(obj);
> +
> +       obj = bpf_object__open_file(file, NULL);
> +       if (CHECK_FAIL(libbpf_get_error(obj)))
> +               return;
> +
> +       err = bpf_object__load(obj);
> +       CHECK(err, "default load", "err %d errno %d\n", err, errno);
> +
> +       /* check that same map ID was reused for second load */
> +       map_id2 = get_map_id(obj, "pinmap");
> +       CHECK(map_id != map_id2, "check reuse",
> +             "err %d errno %d id %d id2 %d\n", err, errno, map_id, map_id2);
> +       unlink(pinpath);
> +       bpf_object__close(obj);
> +
> +       err = mkdir(custpath, 0700);
> +       CHECK(err, "mkdir custpath",  "err %d errno %d\n", err, errno);
> +
> +       obj = bpf_object__open_file(file, &opts);
> +       if (CHECK_FAIL(libbpf_get_error(obj)))
> +               return;
> +
> +       err = bpf_object__load(obj);
> +       CHECK(err, "custom load", "err %d errno %d\n", err, errno);
> +
> +       /* check that pinmap was pinned at the custom path */
> +       err = stat(custpinpath, &statbuf);
> +       CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno);
> +
> +       unlink(custpinpath);
> +       rmdir(custpath);
> +       bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
> new file mode 100644
> index 000000000000..ff2d7447777e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_pinning.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include "bpf_helpers.h"
> +
> +int _version SEC("version") = 1;
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_ARRAY);
> +       __uint(max_entries, 1);
> +       __type(key, __u32);
> +       __type(value, __u64);
> +       __uint(pinning, LIBBPF_PIN_BY_NAME);
> +} pinmap SEC(".maps");
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_ARRAY);
> +       __uint(max_entries, 1);
> +       __type(key, __u32);
> +       __type(value, __u64);
> +} nopinmap SEC(".maps");
> +
> +SEC("xdp_prog")
> +int _xdp_prog(struct xdp_md *xdp)
> +{
> +       return XDP_PASS;
> +}
> +
> +char _license[] SEC("license") = "GPL";
>

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

[Toke Høiland-Jørgensen]

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

> On Sun, Oct 27, 2019 at 1:53 PM 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).
>>
>> Acked-by: Andrii Nakryiko <andriin@fb.com>
>> 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 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).

>> +               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
>
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!map->pin_path && !path) {
>> +               pr_warn("missing pin path\n");
>
> and here?
>
>> +               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...

>> +
>> +       err = check_path(map->pin_path);
>> +       if (err)
>> +               return err;
>> +
>
> [...]
>
>>
>> +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.

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

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

-Toke

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

[Toke Høiland-Jørgensen]

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

> On Sun, Oct 27, 2019 at 1:53 PM 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, 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 <toke@redhat.com>
>> ---
>>  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(-)
>>
>
> [...]
>
>>
>> -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...

>> +
>> +               len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
>> +               if (len < 0)
>> +                       return -EINVAL;
>> +               else if (len >= PATH_MAX)
>
> [...]
>
>>         return 0;
>>  }
>>
>> +static bool map_is_reuse_compat(const struct bpf_map *map,
>> +                               int map_fd)
>
> nit: this should fit on single line?
>
>> +{
>> +       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.

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

>> +       }
>> +
>> +       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;
>> +}
>> +
>
> [...]
>
>> +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.

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

[Andrii Nakryiko]

On Tue, Oct 29, 2019 at 2:01 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Sun, Oct 27, 2019 at 1:53 PM 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).
> >>
> >> Acked-by: Andrii Nakryiko <andriin@fb.com>
> >> 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 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).
>
> >> +               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
> >
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       if (!map->pin_path && !path) {
> >> +               pr_warn("missing pin path\n");
> >
> > and here?
> >
> >> +               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...
>
> >> +
> >> +       err = check_path(map->pin_path);
> >> +       if (err)
> >> +               return err;
> >> +
> >
> > [...]
> >
> >>
> >> +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.
>
> >> +
> >> +       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?

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

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

[Andrii Nakryiko]

On Tue, Oct 29, 2019 at 2:30 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Sun, Oct 27, 2019 at 1:53 PM 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, 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 <toke@redhat.com>
> >> ---
> >>  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(-)
> >>
> >
> > [...]
> >
> >>
> >> -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...

Let's now expose more stuff than what we need to expose. If we really
will have a need for that, it's really easy to add. Right now you
won't even need to store pinning attribute in bpf_map, because you'll
be just setting proper pin_path in init_user_maps(), as suggested
above.

>
> >> +
> >> +               len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
> >> +               if (len < 0)
> >> +                       return -EINVAL;
> >> +               else if (len >= PATH_MAX)
> >
> > [...]
> >
> >>         return 0;
> >>  }
> >>
> >> +static bool map_is_reuse_compat(const struct bpf_map *map,
> >> +                               int map_fd)
> >
> > nit: this should fit on single line?
> >
> >> +{
> >> +       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.

type IDs are just an ordered index of a type, as generated by Clang.
No stability guarantees. Just adding extra typedef somewhere in
unrelated type might shift all the type IDs around.

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

I guess I do insist on correct handling of errno, instead of
potentially returning garbage value from some unrelated syscall from
inside of pr_warn's user-provided callback.

Even libbpf_strerror_r can garble errno (e.g., through its strerror_r
call), so make sure you store it before passing into
libbpf_strerror_r().

>
> >> +       }
> >> +
> >> +       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;
> >> +}
> >> +
> >
> > [...]
> >
> >> +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.

What's the benefit of restricting it to just this use case? If app
wants to use something other than /sys/fs/bpf as a default root path,
why would that be restricted only to auto-pinned maps? It seems to me
that having set this on bpf_object__open() and then calling
bpf_object__pin_maps(NULL) should just take this overridden root path
into account. Isn't that a logical behavior?

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

[Toke Høiland-Jørgensen]

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

> On Tue, Oct 29, 2019 at 2:01 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Sun, Oct 27, 2019 at 1:53 PM 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).
>> >>
>> >> Acked-by: Andrii Nakryiko <andriin@fb.com>
>> >> 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 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).
>>
>> >> +               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
>> >
>> >> +               return -EINVAL;
>> >> +       }
>> >> +
>> >> +       if (!map->pin_path && !path) {
>> >> +               pr_warn("missing pin path\n");
>> >
>> > and here?
>> >
>> >> +               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...
>>
>> >> +
>> >> +       err = check_path(map->pin_path);
>> >> +       if (err)
>> >> +               return err;
>> >> +
>> >
>> > [...]
>> >
>> >>
>> >> +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.
>>
>> >> +
>> >> +       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.

Sure, I'm not objecting to making things play nicely with each other to
the largest extent possible. I'm just vary of changing existing
behaviour.

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

Changing something that would previously fail to just silently succeed
does make me a bit twitchy. However, I suppose that as long as we try to
make sure it really is a no-op (i.e., re-pinning a map *in the same
path* can "succeed" silently). Will try something to that effect...

-Toke

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

[Toke Høiland-Jørgensen]

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

> On Tue, Oct 29, 2019 at 2:30 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Sun, Oct 27, 2019 at 1:53 PM 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, 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 <toke@redhat.com>
>> >> ---
>> >>  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(-)
>> >>
>> >
>> > [...]
>> >
>> >>
>> >> -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...
>
> Let's now expose more stuff than what we need to expose. If we really
> will have a need for that, it's really easy to add. Right now you
> won't even need to store pinning attribute in bpf_map, because you'll
> be just setting proper pin_path in init_user_maps(), as suggested
> above.

While I do think it's a bit weird that there's an attribute you can set
but can't get at, I will grudgingly admit that it's not strictly needed
right now... So OK, I'll leave it out :)

>> >> +
>> >> +               len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
>> >> +               if (len < 0)
>> >> +                       return -EINVAL;
>> >> +               else if (len >= PATH_MAX)
>> >
>> > [...]
>> >
>> >>         return 0;
>> >>  }
>> >>
>> >> +static bool map_is_reuse_compat(const struct bpf_map *map,
>> >> +                               int map_fd)
>> >
>> > nit: this should fit on single line?
>> >
>> >> +{
>> >> +       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.
>
> type IDs are just an ordered index of a type, as generated by Clang.
> No stability guarantees. Just adding extra typedef somewhere in
> unrelated type might shift all the type IDs around.

Ah, so it's just numbering types within the same translation unit? I
thought it was somehow globally (or system-wide) unique (though not sure
how I imagined that would be achieved, TBH).

>> >> +}
>> >> +
>> >> +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...
>
> I guess I do insist on correct handling of errno, instead of
> potentially returning garbage value from some unrelated syscall from
> inside of pr_warn's user-provided callback.
>
> Even libbpf_strerror_r can garble errno (e.g., through its strerror_r
> call), so make sure you store it before passing into
> libbpf_strerror_r().

Ohh, right, didn't think about those having side effects; then your
worry makes more sense. I thought you were just being pedantic, which is
why I was being grumpy (did change it, though) :)

>>
>> >> +       }
>> >> +
>> >> +       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;
>> >> +}
>> >> +
>> >
>> > [...]
>> >
>> >> +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.
>
> What's the benefit of restricting it to just this use case? If app
> wants to use something other than /sys/fs/bpf as a default root path,
> why would that be restricted only to auto-pinned maps? It seems to me
> that having set this on bpf_object__open() and then calling
> bpf_object__pin_maps(NULL) should just take this overridden root path
> into account. Isn't that a logical behavior?

No, I think the logical behaviour is for pin_maps(NULL) to just pin all
maps at map->pin_path if set (and same for unpin). Already changed it to
this behaviour, actually.

-Toke

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

[Andrii Nakryiko]

On Tue, Oct 29, 2019 at 11:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Tue, Oct 29, 2019 at 2:30 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >> > On Sun, Oct 27, 2019 at 1:53 PM 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, 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 <toke@redhat.com>
> >> >> ---
> >> >>  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(-)
> >> >>
> >> >
> >> > [...]
> >> >
> >> >>
> >> >> -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...
> >
> > Let's now expose more stuff than what we need to expose. If we really
> > will have a need for that, it's really easy to add. Right now you
> > won't even need to store pinning attribute in bpf_map, because you'll
> > be just setting proper pin_path in init_user_maps(), as suggested
> > above.
>
> While I do think it's a bit weird that there's an attribute you can set
> but can't get at, I will grudgingly admit that it's not strictly needed
> right now... So OK, I'll leave it out :)
>
> >> >> +
> >> >> +               len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
> >> >> +               if (len < 0)
> >> >> +                       return -EINVAL;
> >> >> +               else if (len >= PATH_MAX)
> >> >
> >> > [...]
> >> >
> >> >>         return 0;
> >> >>  }
> >> >>
> >> >> +static bool map_is_reuse_compat(const struct bpf_map *map,
> >> >> +                               int map_fd)
> >> >
> >> > nit: this should fit on single line?
> >> >
> >> >> +{
> >> >> +       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.
> >
> > type IDs are just an ordered index of a type, as generated by Clang.
> > No stability guarantees. Just adding extra typedef somewhere in
> > unrelated type might shift all the type IDs around.
>
> Ah, so it's just numbering types within the same translation unit? I
> thought it was somehow globally (or system-wide) unique (though not sure
> how I imagined that would be achieved, TBH).
>
> >> >> +}
> >> >> +
> >> >> +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...
> >
> > I guess I do insist on correct handling of errno, instead of
> > potentially returning garbage value from some unrelated syscall from
> > inside of pr_warn's user-provided callback.
> >
> > Even libbpf_strerror_r can garble errno (e.g., through its strerror_r
> > call), so make sure you store it before passing into
> > libbpf_strerror_r().
>
> Ohh, right, didn't think about those having side effects; then your
> worry makes more sense. I thought you were just being pedantic, which is
> why I was being grumpy (did change it, though) :)
>
> >>
> >> >> +       }
> >> >> +
> >> >> +       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;
> >> >> +}
> >> >> +
> >> >
> >> > [...]
> >> >
> >> >> +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.
> >
> > What's the benefit of restricting it to just this use case? If app
> > wants to use something other than /sys/fs/bpf as a default root path,
> > why would that be restricted only to auto-pinned maps? It seems to me
> > that having set this on bpf_object__open() and then calling
> > bpf_object__pin_maps(NULL) should just take this overridden root path
> > into account. Isn't that a logical behavior?
>
> No, I think the logical behaviour is for pin_maps(NULL) to just pin all
> maps at map->pin_path if set (and same for unpin). Already changed it to
> this behaviour, actually.

Sure, I guess that makes sense as well. Can you please add comment to
pin_maps describing this convention?

I still think that auto_pin_path is both too specific and also
imprecise at the same time :) It's not really a pin path, it's a part
of a pin path for any particular map, it's a root of those paths. And
let's not corner us to just auto-pinning use case yet by saying it's
auto_pin_path? So something like pin_root_path or root_pin_path is
generic enough to allow extensions if we need them, but without being
misleading.

>
> -Toke

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

[Toke Høiland-Jørgensen]

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

> On Tue, Oct 29, 2019 at 11:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Tue, Oct 29, 2019 at 2:30 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>> >>
>> >> > On Sun, Oct 27, 2019 at 1:53 PM 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, 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 <toke@redhat.com>
>> >> >> ---
>> >> >>  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(-)
>> >> >>
>> >> >
>> >> > [...]
>> >> >
>> >> >>
>> >> >> -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...
>> >
>> > Let's now expose more stuff than what we need to expose. If we really
>> > will have a need for that, it's really easy to add. Right now you
>> > won't even need to store pinning attribute in bpf_map, because you'll
>> > be just setting proper pin_path in init_user_maps(), as suggested
>> > above.
>>
>> While I do think it's a bit weird that there's an attribute you can set
>> but can't get at, I will grudgingly admit that it's not strictly needed
>> right now... So OK, I'll leave it out :)
>>
>> >> >> +
>> >> >> +               len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
>> >> >> +               if (len < 0)
>> >> >> +                       return -EINVAL;
>> >> >> +               else if (len >= PATH_MAX)
>> >> >
>> >> > [...]
>> >> >
>> >> >>         return 0;
>> >> >>  }
>> >> >>
>> >> >> +static bool map_is_reuse_compat(const struct bpf_map *map,
>> >> >> +                               int map_fd)
>> >> >
>> >> > nit: this should fit on single line?
>> >> >
>> >> >> +{
>> >> >> +       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.
>> >
>> > type IDs are just an ordered index of a type, as generated by Clang.
>> > No stability guarantees. Just adding extra typedef somewhere in
>> > unrelated type might shift all the type IDs around.
>>
>> Ah, so it's just numbering types within the same translation unit? I
>> thought it was somehow globally (or system-wide) unique (though not sure
>> how I imagined that would be achieved, TBH).
>>
>> >> >> +}
>> >> >> +
>> >> >> +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...
>> >
>> > I guess I do insist on correct handling of errno, instead of
>> > potentially returning garbage value from some unrelated syscall from
>> > inside of pr_warn's user-provided callback.
>> >
>> > Even libbpf_strerror_r can garble errno (e.g., through its strerror_r
>> > call), so make sure you store it before passing into
>> > libbpf_strerror_r().
>>
>> Ohh, right, didn't think about those having side effects; then your
>> worry makes more sense. I thought you were just being pedantic, which is
>> why I was being grumpy (did change it, though) :)
>>
>> >>
>> >> >> +       }
>> >> >> +
>> >> >> +       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;
>> >> >> +}
>> >> >> +
>> >> >
>> >> > [...]
>> >> >
>> >> >> +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.
>> >
>> > What's the benefit of restricting it to just this use case? If app
>> > wants to use something other than /sys/fs/bpf as a default root path,
>> > why would that be restricted only to auto-pinned maps? It seems to me
>> > that having set this on bpf_object__open() and then calling
>> > bpf_object__pin_maps(NULL) should just take this overridden root path
>> > into account. Isn't that a logical behavior?
>>
>> No, I think the logical behaviour is for pin_maps(NULL) to just pin all
>> maps at map->pin_path if set (and same for unpin). Already changed it to
>> this behaviour, actually.
>
> Sure, I guess that makes sense as well. Can you please add comment to
> pin_maps describing this convention?

Can do. Do you mean as documentation in libbpf.h, or just above the
function in libbpf.c?

> I still think that auto_pin_path is both too specific and also
> imprecise at the same time :) It's not really a pin path, it's a part
> of a pin path for any particular map, it's a root of those paths. And
> let's not corner us to just auto-pinning use case yet by saying it's
> auto_pin_path? So something like pin_root_path or root_pin_path is
> generic enough to allow extensions if we need them, but without being
> misleading.

Oh sure, don't mind changing the option name :)

-Toke