bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v4 0/5] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute
@ 2019-10-29 19:39 Toke Høiland-Jørgensen
  2019-10-29 19:39 ` [PATCH bpf-next v4 1/5] libbpf: Fix error handling in bpf_map__reuse_fd() Toke Høiland-Jørgensen
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-29 19:39 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
	Jesper Dangaard Brouer, Andrii Nakryiko, David Miller, netdev,
	bpf

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:

v4:
  - Don't check key_type_id and value_type_id when checking for map reuse
    compatibility.
  - Move building of map->pin_path into init_user_btf_map()
  - Get rid of 'pinning' attribute in struct bpf_map
  - Make sure we also create parent directory on auto-pin (new patch 3).
  - Abort the selftest on error instead of attempting to continue.
  - Support unpinning all pinned maps with bpf_object__unpin_maps(obj, NULL)
  - Support pinning at map->pin_path with bpf_object__pin_maps(obj, NULL)
  - Make re-pinning a map at the same path a noop
  - Rename the open option to pin_root_path
  - Add a bunch more self-tests for pin_maps(NULL) and unpin_maps(NULL)
  - Fix a couple of smaller nits

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 (5):
      libbpf: Fix error handling in bpf_map__reuse_fd()
      libbpf: Store map pin path and status in struct bpf_map
      libbpf: Move directory creation into _pin() functions
      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                           |  383 +++++++++++++++++-----
 tools/lib/bpf/libbpf.h                           |   21 +
 tools/lib/bpf/libbpf.map                         |    3 
 tools/testing/selftests/bpf/prog_tests/pinning.c |  157 +++++++++
 tools/testing/selftests/bpf/progs/test_pinning.c |   29 ++
 6 files changed, 518 insertions(+), 81 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/pinning.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_pinning.c


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH bpf-next v4 1/5] libbpf: Fix error handling in bpf_map__reuse_fd()
  2019-10-29 19:39 [PATCH bpf-next v4 0/5] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
@ 2019-10-29 19:39 ` Toke Høiland-Jørgensen
  2019-10-29 19:39 ` [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map Toke Høiland-Jørgensen
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-29 19:39 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
	Jesper Dangaard Brouer, Andrii Nakryiko, David Miller, netdev,
	bpf

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)


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map
  2019-10-29 19:39 [PATCH bpf-next v4 0/5] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
  2019-10-29 19:39 ` [PATCH bpf-next v4 1/5] libbpf: Fix error handling in bpf_map__reuse_fd() Toke Høiland-Jørgensen
@ 2019-10-29 19:39 ` Toke Høiland-Jørgensen
  2019-10-31 17:22   ` Andrii Nakryiko
  2019-10-29 19:39 ` [PATCH bpf-next v4 3/5] libbpf: Move directory creation into _pin() functions Toke Høiland-Jørgensen
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-29 19:39 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
	Jesper Dangaard Brouer, Andrii Nakryiko, David Miller, netdev,
	bpf

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

The behaviour of bpf_object__{un,}pin_maps() is changed so that if it is
called with a NULL path argument (which was previously illegal), it will
(un)pin only those maps that have a pin_path set.

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ce5ef3ddd263..fd11f6aeb32c 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,119 @@ 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->pin_path) {
+		if (path && strcmp(path, map->pin_path)) {
+			pr_warn("map '%s' already has pin path '%s' different from '%s'\n",
+				bpf_map__name(map), map->pin_path, path);
+			return -EINVAL;
+		} else if (map->pinned) {
+			pr_debug("map '%s' already pinned at '%s'; not re-pinning\n",
+				 bpf_map__name(map), map->pin_path);
+			return 0;
+		}
+	} else {
+		if (!path) {
+			pr_warn("missing a path to pin map '%s' at\n",
+				bpf_map__name(map));
+			return -EINVAL;
+		} else if (map->pinned) {
+			pr_warn("map '%s' already pinned\n", bpf_map__name(map));
+			return -EEXIST;
+		}
+
+		map->pin_path = strdup(path);
+		if (!map->pin_path) {
+			err = -errno;
+			goto out_err;
+		}
 	}
 
-	pr_debug("pinned map '%s'\n", path);
+	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;
 	}
 
+	if (map->pin_path) {
+		if (path && strcmp(path, map->pin_path)) {
+			pr_warn("map '%s' already has pin path '%s' different from '%s'\n",
+				bpf_map__name(map), map->pin_path, path);
+			return -EINVAL;
+		}
+		path = map->pin_path;
+	} else if (!path) {
+		pr_warn("no path to unpin map '%s' from\n",
+			bpf_map__name(map));
+		return -EINVAL;
+	}
+
+	err = check_path(path);
+	if (err)
+		return err;
+
 	err = unlink(path);
 	if (err != 0)
 		return -errno;
-	pr_debug("unpinned map '%s'\n", path);
+
+	map->pinned = false;
+	pr_debug("unpinned map from '%s' from '%s'\n", bpf_map__name(map), path);
 
 	return 0;
 }
 
+int bpf_map__set_pin_path(struct bpf_map *map, const char *path)
+{
+	char *new = NULL;
+
+	if (path) {
+		new = strdup(path);
+		if (!new)
+			return -errno;
+	}
+
+	free(map->pin_path);
+	map->pin_path = new;
+	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;
@@ -4084,20 +4158,27 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 		return err;
 
 	bpf_object__for_each_map(map, obj) {
+		char *pin_path = NULL;
 		char buf[PATH_MAX];
-		int len;
 
-		len = snprintf(buf, PATH_MAX, "%s/%s", path,
-			       bpf_map__name(map));
-		if (len < 0) {
-			err = -EINVAL;
-			goto err_unpin_maps;
-		} else if (len >= PATH_MAX) {
-			err = -ENAMETOOLONG;
-			goto err_unpin_maps;
+		if (path) {
+			int len;
+
+			len = snprintf(buf, PATH_MAX, "%s/%s", path,
+				       bpf_map__name(map));
+			if (len < 0) {
+				err = -EINVAL;
+				goto err_unpin_maps;
+			} else if (len >= PATH_MAX) {
+				err = -ENAMETOOLONG;
+				goto err_unpin_maps;
+			}
+			pin_path = buf;
+		} else if (!map->pin_path) {
+			continue;
 		}
 
-		err = bpf_map__pin(map, buf);
+		err = bpf_map__pin(map, pin_path);
 		if (err)
 			goto err_unpin_maps;
 	}
@@ -4106,17 +4187,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;
@@ -4131,17 +4205,24 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
 		return -ENOENT;
 
 	bpf_object__for_each_map(map, obj) {
+		char *pin_path = NULL;
 		char buf[PATH_MAX];
-		int len;
 
-		len = snprintf(buf, PATH_MAX, "%s/%s", path,
-			       bpf_map__name(map));
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (path) {
+			int len;
+
+			len = snprintf(buf, PATH_MAX, "%s/%s", path,
+				       bpf_map__name(map));
+			if (len < 0)
+				return -EINVAL;
+			else if (len >= PATH_MAX)
+				return -ENAMETOOLONG;
+			pin_path = buf;
+		} else if (!map->pin_path) {
+			continue;
+		}
 
-		err = bpf_map__unpin(map, buf);
+		err = bpf_map__unpin(map, pin_path);
 		if (err)
 			return err;
 	}
@@ -4266,6 +4347,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..e28ef2ebe062 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -124,6 +124,11 @@ 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);
+
+/* pin_maps and unpin_maps can both be called with a NULL path, in which case
+ * they will use the pin_path attribute of each map (and ignore all maps that
+ * don't have a pin_path set).
+ */
 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);
@@ -385,6 +390,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;


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH bpf-next v4 3/5] libbpf: Move directory creation into _pin() functions
  2019-10-29 19:39 [PATCH bpf-next v4 0/5] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
  2019-10-29 19:39 ` [PATCH bpf-next v4 1/5] libbpf: Fix error handling in bpf_map__reuse_fd() Toke Høiland-Jørgensen
  2019-10-29 19:39 ` [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map Toke Høiland-Jørgensen
@ 2019-10-29 19:39 ` Toke Høiland-Jørgensen
  2019-10-31 17:27   ` Andrii Nakryiko
  2019-10-29 19:39 ` [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects Toke Høiland-Jørgensen
  2019-10-29 19:39 ` [PATCH bpf-next v4 5/5] selftests: Add tests for automatic map pinning Toke Høiland-Jørgensen
  4 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-29 19:39 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
	Jesper Dangaard Brouer, Andrii Nakryiko, David Miller, netdev,
	bpf

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

The existing pin_*() functions all try to create the parent directory
before pinning. Move this check into the per-object _pin() functions
instead. This ensures consistent behaviour when auto-pinning is
added (which doesn't go through the top-level pin_maps() function), at the
cost of a few more calls to mkdir().

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fd11f6aeb32c..895066393508 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3805,6 +3805,28 @@ int bpf_object__load(struct bpf_object *obj)
 	return bpf_object__load_xattr(&attr);
 }
 
+static int make_parent_dir(const char *path)
+{
+	char *cp, errmsg[STRERR_BUFSIZE];
+	char *dname, *dir;
+	int err = 0;
+
+	dname = strdup(path);
+	if (dname == NULL)
+		return -ENOMEM;
+
+	dir = dirname(dname);
+	if (mkdir(dir, 0700) && errno != EEXIST)
+		err = -errno;
+
+	free(dname);
+	if (err) {
+		cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg));
+		pr_warn("failed to mkdir %s: %s\n", path, cp);
+	}
+	return err;
+}
+
 static int check_path(const char *path)
 {
 	char *cp, errmsg[STRERR_BUFSIZE];
@@ -3841,6 +3863,10 @@ int bpf_program__pin_instance(struct bpf_program *prog, const char *path,
 	char *cp, errmsg[STRERR_BUFSIZE];
 	int err;
 
+	err = make_parent_dir(path);
+	if (err)
+		return err;
+
 	err = check_path(path);
 	if (err)
 		return err;
@@ -3894,25 +3920,14 @@ int bpf_program__unpin_instance(struct bpf_program *prog, const char *path,
 	return 0;
 }
 
-static int make_dir(const char *path)
-{
-	char *cp, errmsg[STRERR_BUFSIZE];
-	int err = 0;
-
-	if (mkdir(path, 0700) && errno != EEXIST)
-		err = -errno;
-
-	if (err) {
-		cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg));
-		pr_warn("failed to mkdir %s: %s\n", path, cp);
-	}
-	return err;
-}
-
 int bpf_program__pin(struct bpf_program *prog, const char *path)
 {
 	int i, err;
 
+	err = make_parent_dir(path);
+	if (err)
+		return err;
+
 	err = check_path(path);
 	if (err)
 		return err;
@@ -3933,10 +3948,6 @@ int bpf_program__pin(struct bpf_program *prog, const char *path)
 		return bpf_program__pin_instance(prog, path, 0);
 	}
 
-	err = make_dir(path);
-	if (err)
-		return err;
-
 	for (i = 0; i < prog->instances.nr; i++) {
 		char buf[PATH_MAX];
 		int len;
@@ -4059,6 +4070,10 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
 		}
 	}
 
+	err = make_parent_dir(map->pin_path);
+	if (err)
+		return err;
+
 	err = check_path(map->pin_path);
 	if (err)
 		return err;
@@ -4153,10 +4168,6 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 		return -ENOENT;
 	}
 
-	err = make_dir(path);
-	if (err)
-		return err;
-
 	bpf_object__for_each_map(map, obj) {
 		char *pin_path = NULL;
 		char buf[PATH_MAX];
@@ -4243,10 +4254,6 @@ int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
 		return -ENOENT;
 	}
 
-	err = make_dir(path);
-	if (err)
-		return err;
-
 	bpf_object__for_each_program(prog, obj) {
 		char buf[PATH_MAX];
 		int len;


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects
  2019-10-29 19:39 [PATCH bpf-next v4 0/5] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
                   ` (2 preceding siblings ...)
  2019-10-29 19:39 ` [PATCH bpf-next v4 3/5] libbpf: Move directory creation into _pin() functions Toke Høiland-Jørgensen
@ 2019-10-29 19:39 ` Toke Høiland-Jørgensen
  2019-10-31 17:37   ` Andrii Nakryiko
  2019-10-29 19:39 ` [PATCH bpf-next v4 5/5] selftests: Add tests for automatic map pinning Toke Høiland-Jørgensen
  4 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-29 19:39 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
	Jesper Dangaard Brouer, Andrii Nakryiko, David Miller, netdev,
	bpf

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      |  144 +++++++++++++++++++++++++++++++++++++++++--
 tools/lib/bpf/libbpf.h      |   13 ++++
 3 files changed, 154 insertions(+), 9 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 895066393508..2b0a57edbaf9 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1092,10 +1092,32 @@ static bool get_map_field_int(const char *map_name, const struct btf *btf,
 	return true;
 }
 
+static int build_map_pin_path(struct bpf_map *map, const char *path)
+{
+	char buf[PATH_MAX];
+	int err, len;
+
+	if (!path)
+		path = "/sys/fs/bpf";
+
+	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_user_btf_map(struct bpf_object *obj,
 					 const struct btf_type *sec,
 					 int var_idx, int sec_idx,
-					 const Elf_Data *data, bool strict)
+					 const Elf_Data *data, bool strict,
+					 const char *pin_root_path)
 {
 	const struct btf_type *var, *def, *t;
 	const struct btf_var_secinfo *vi;
@@ -1270,6 +1292,28 @@ 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;
+			int err;
+
+			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;
+			}
+			err = build_map_pin_path(map, pin_root_path);
+			if (err) {
+				pr_warn("map '%s': couldn't build pin path.\n",
+					map_name);
+				return err;
+			}
 		} else {
 			if (strict) {
 				pr_warn("map '%s': unknown field '%s'.\n",
@@ -1289,7 +1333,8 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
 	return 0;
 }
 
-static int bpf_object__init_user_btf_maps(struct bpf_object *obj, bool strict)
+static int bpf_object__init_user_btf_maps(struct bpf_object *obj, bool strict,
+					  const char *pin_root_path)
 {
 	const struct btf_type *sec = NULL;
 	int nr_types, i, vlen, err;
@@ -1331,7 +1376,7 @@ static int bpf_object__init_user_btf_maps(struct bpf_object *obj, bool strict)
 	for (i = 0; i < vlen; i++) {
 		err = bpf_object__init_user_btf_map(obj, sec, i,
 						    obj->efile.btf_maps_shndx,
-						    data, strict);
+						    data, strict, pin_root_path);
 		if (err)
 			return err;
 	}
@@ -1339,7 +1384,8 @@ 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__init_maps(struct bpf_object *obj, bool relaxed_maps,
+				 const char *pin_root_path)
 {
 	bool strict = !relaxed_maps;
 	int err;
@@ -1348,7 +1394,7 @@ static int bpf_object__init_maps(struct bpf_object *obj, bool relaxed_maps)
 	if (err)
 		return err;
 
-	err = bpf_object__init_user_btf_maps(obj, strict);
+	err = bpf_object__init_user_btf_maps(obj, strict, pin_root_path);
 	if (err)
 		return err;
 
@@ -1537,7 +1583,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 *pin_root_path)
 {
 	Elf *elf = obj->efile.elf;
 	GElf_Ehdr *ep = &obj->efile.ehdr;
@@ -1672,7 +1719,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, pin_root_path);
 	if (!err)
 		err = bpf_object__sanitize_and_load_btf(obj);
 	if (!err)
@@ -2128,6 +2175,66 @@ 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);
+}
+
+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) {
+		err = -errno;
+		if (err == -ENOENT) {
+			pr_debug("found no pinned map to reuse at '%s'\n",
+				 map->pin_path);
+			return 0;
+		}
+
+		cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg));
+		pr_warn("couldn't retrieve pinned map '%s': %s\n",
+			map->pin_path, cp);
+		return err;
+	}
+
+	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 +2277,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 +2364,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 +3744,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 *pin_root_path;
 	struct bpf_program *prog;
 	struct bpf_object *obj;
 	const char *obj_name;
@@ -3653,11 +3779,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);
+	pin_root_path = OPTS_GET(opts, pin_root_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, pin_root_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 e28ef2ebe062..d9a685d20f7a 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -103,8 +103,13 @@ struct bpf_object_open_opts {
 	bool relaxed_maps;
 	/* process CO-RE relocations non-strictly, allowing them to fail */
 	bool relaxed_core_relocs;
+	/* maps that set the 'pinning' attribute in their definition will have
+	 * their pin_path attribute set to a file in this directory, and be
+	 * auto-pinned to that path on load; defaults to "/sys/fs/bpf".
+	 */
+	const char *pin_root_path;
 };
-#define bpf_object_open_opts__last_field relaxed_core_relocs
+#define bpf_object_open_opts__last_field pin_root_path
 
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
@@ -125,6 +130,12 @@ int bpf_object__section_size(const struct bpf_object *obj, const char *name,
 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,
+};
+
 /* pin_maps and unpin_maps can both be called with a NULL path, in which case
  * they will use the pin_path attribute of each map (and ignore all maps that
  * don't have a pin_path set).


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH bpf-next v4 5/5] selftests: Add tests for automatic map pinning
  2019-10-29 19:39 [PATCH bpf-next v4 0/5] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
                   ` (3 preceding siblings ...)
  2019-10-29 19:39 ` [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects Toke Høiland-Jørgensen
@ 2019-10-29 19:39 ` Toke Høiland-Jørgensen
  2019-10-31 18:02   ` Andrii Nakryiko
  4 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-29 19:39 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
	Jesper Dangaard Brouer, Andrii Nakryiko, David Miller, netdev,
	bpf

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 |  157 ++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_pinning.c |   29 ++++
 2 files changed, 186 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..71f7dc51edc7
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
@@ -0,0 +1,157 @@
+// 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"))
+		return 0;
+
+	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;
+}
+
+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;
+	struct bpf_map *map;
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+		.pin_root_path = custpath,
+	);
+
+	int err;
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK_FAIL(libbpf_get_error(obj)))
+		return;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "default load", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* check that pinmap was pinned */
+	err = stat(pinpath, &statbuf);
+	if (CHECK(err, "stat pinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
+        /* check that nopinmap was *not* pinned */
+	err = stat(nopinpath, &statbuf);
+	if (CHECK(!err || errno != ENOENT, "stat nopinpath",
+		  "err %d errno %d\n", err, errno))
+		goto out;
+
+        map_id = get_map_id(obj, "pinmap");
+	if (!map_id)
+		goto out;
+
+	bpf_object__close(obj);
+
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK_FAIL(libbpf_get_error(obj)))
+		goto out;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "default load", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* check that same map ID was reused for second load */
+	map_id2 = get_map_id(obj, "pinmap");
+	if (CHECK(map_id != map_id2, "check reuse",
+		  "err %d errno %d id %d id2 %d\n", err, errno, map_id, map_id2))
+		goto out;
+
+	/* should be no-op to re-pin same map */
+	map = bpf_object__find_map_by_name(obj, "pinmap");
+	if (CHECK(!map, "find map", "NULL map"))
+		goto out;
+
+	err = bpf_map__pin(map, NULL);
+	if (CHECK(err, "re-pin map", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* but error to pin at different location */
+	err = bpf_map__pin(map, "/sys/fs/bpf/other");
+	if (CHECK(!err, "pin map different", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* unpin maps with a pin_path set */
+	err = bpf_object__unpin_maps(obj, NULL);
+	if (CHECK(err, "unpin maps", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* and re-pin them... */
+	err = bpf_object__pin_maps(obj, NULL);
+	if (CHECK(err, "pin maps", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* set pinning path of other map and re-pin all */
+	map = bpf_object__find_map_by_name(obj, "nopinmap");
+	if (CHECK(!map, "find map", "NULL map"))
+		goto out;
+
+	err = bpf_map__set_pin_path(map, custpinpath);
+	if (CHECK(err, "set pin path", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* should only pin the one unpinned map */
+	err = bpf_object__pin_maps(obj, NULL);
+	if (CHECK(err, "pin maps", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* check that nopinmap was pinned at the custom path */
+	err = stat(custpinpath, &statbuf);
+	if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* remove the custom pin path to re-test it with auto-pinning below */
+	err = unlink(custpinpath);
+	if (CHECK(err, "unlink custpinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
+	err = rmdir(custpath);
+	if (CHECK(err, "rmdir custpindir", "err %d errno %d\n", err, errno))
+		goto out;
+
+	bpf_object__close(obj);
+
+	/* test auto-pinning at custom path with open opt */
+	obj = bpf_object__open_file(file, &opts);
+	if (CHECK_FAIL(libbpf_get_error(obj)))
+		return;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "custom load", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* check that pinmap was pinned at the custom path */
+	err = stat(custpinpath, &statbuf);
+	if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
+out:
+	unlink(pinpath);
+	unlink(nopinpath);
+	unlink(custpinpath);
+	rmdir(custpath);
+	if (obj)
+		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";


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map
  2019-10-29 19:39 ` [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map Toke Høiland-Jørgensen
@ 2019-10-31 17:22   ` Andrii Nakryiko
  2019-10-31 17:26     ` Toke Høiland-Jørgensen
  2019-10-31 17:31     ` Toke Høiland-Jørgensen
  0 siblings, 2 replies; 17+ messages in thread
From: Andrii Nakryiko @ 2019-10-31 17:22 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

On Tue, Oct 29, 2019 at 12:39 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).
>
> The behaviour of bpf_object__{un,}pin_maps() is changed so that if it is
> called with a NULL path argument (which was previously illegal), it will
> (un)pin only those maps that have a pin_path set.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Looks good, thanks! Just some minor things to fix up below.

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

>  tools/lib/bpf/libbpf.c   |  164 +++++++++++++++++++++++++++++++++++-----------
>  tools/lib/bpf/libbpf.h   |    8 ++
>  tools/lib/bpf/libbpf.map |    3 +
>  3 files changed, 134 insertions(+), 41 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ce5ef3ddd263..fd11f6aeb32c 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,119 @@ 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->pin_path) {
> +               if (path && strcmp(path, map->pin_path)) {
> +                       pr_warn("map '%s' already has pin path '%s' different from '%s'\n",
> +                               bpf_map__name(map), map->pin_path, path);
> +                       return -EINVAL;
> +               } else if (map->pinned) {
> +                       pr_debug("map '%s' already pinned at '%s'; not re-pinning\n",
> +                                bpf_map__name(map), map->pin_path);
> +                       return 0;
> +               }

`if (map->pinned)` check is the same in both branches, so I'd do it
first, before this map->pin_path if/else.

> +       } else {
> +               if (!path) {
> +                       pr_warn("missing a path to pin map '%s' at\n",
> +                               bpf_map__name(map));
> +                       return -EINVAL;
> +               } else if (map->pinned) {
> +                       pr_warn("map '%s' already pinned\n", bpf_map__name(map));
> +                       return -EEXIST;
> +               }
> +
> +               map->pin_path = strdup(path);
> +               if (!map->pin_path) {
> +                       err = -errno;
> +                       goto out_err;
> +               }
>         }
>

[...]

> +
> +       err = check_path(path);
> +       if (err)
> +               return err;
> +
>         err = unlink(path);
>         if (err != 0)
>                 return -errno;
> -       pr_debug("unpinned map '%s'\n", path);
> +
> +       map->pinned = false;
> +       pr_debug("unpinned map from '%s' from '%s'\n", bpf_map__name(map), path);

typo: extra from before map name?

>
>         return 0;
>  }
>

[...]

>
>         return err;
> @@ -4131,17 +4205,24 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
>                 return -ENOENT;
>
>         bpf_object__for_each_map(map, obj) {
> +               char *pin_path = NULL;
>                 char buf[PATH_MAX];

you can call buf as pin_path and get rid of extra pointer?

> -               int len;
>
> -               len = snprintf(buf, PATH_MAX, "%s/%s", path,
> -                              bpf_map__name(map));
> -               if (len < 0)
> -                       return -EINVAL;
> -               else if (len >= PATH_MAX)
> -                       return -ENAMETOOLONG;
> +               if (path) {
> +                       int len;
> +
> +                       len = snprintf(buf, PATH_MAX, "%s/%s", path,
> +                                      bpf_map__name(map));
> +                       if (len < 0)
> +                               return -EINVAL;
> +                       else if (len >= PATH_MAX)
> +                               return -ENAMETOOLONG;
> +                       pin_path = buf;
> +               } else if (!map->pin_path) {
> +                       continue;
> +               }
>
> -               err = bpf_map__unpin(map, buf);
> +               err = bpf_map__unpin(map, pin_path);
>                 if (err)
>                         return err;
>         }

[...]

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

we try to keep this list alphabetically sorted

>  } LIBBPF_0.0.5;
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map
  2019-10-31 17:22   ` Andrii Nakryiko
@ 2019-10-31 17:26     ` Toke Høiland-Jørgensen
  2019-10-31 17:28       ` Andrii Nakryiko
  2019-10-31 17:31     ` Toke Høiland-Jørgensen
  1 sibling, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-31 17:26 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

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

> On Tue, Oct 29, 2019 at 12:39 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).
>>
>> The behaviour of bpf_object__{un,}pin_maps() is changed so that if it is
>> called with a NULL path argument (which was previously illegal), it will
>> (un)pin only those maps that have a pin_path set.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> Looks good, thanks! Just some minor things to fix up below.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
>>  tools/lib/bpf/libbpf.c   |  164 +++++++++++++++++++++++++++++++++++-----------
>>  tools/lib/bpf/libbpf.h   |    8 ++
>>  tools/lib/bpf/libbpf.map |    3 +
>>  3 files changed, 134 insertions(+), 41 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index ce5ef3ddd263..fd11f6aeb32c 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,119 @@ 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->pin_path) {
>> +               if (path && strcmp(path, map->pin_path)) {
>> +                       pr_warn("map '%s' already has pin path '%s' different from '%s'\n",
>> +                               bpf_map__name(map), map->pin_path, path);
>> +                       return -EINVAL;
>> +               } else if (map->pinned) {
>> +                       pr_debug("map '%s' already pinned at '%s'; not re-pinning\n",
>> +                                bpf_map__name(map), map->pin_path);
>> +                       return 0;
>> +               }
>
> `if (map->pinned)` check is the same in both branches, so I'd do it
> first, before this map->pin_path if/else.

But it's not. It's debug & return if pin_path is set, and an error
otherwise.

Will fix the rest of your nits :)

-Toke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 3/5] libbpf: Move directory creation into _pin() functions
  2019-10-29 19:39 ` [PATCH bpf-next v4 3/5] libbpf: Move directory creation into _pin() functions Toke Høiland-Jørgensen
@ 2019-10-31 17:27   ` Andrii Nakryiko
  0 siblings, 0 replies; 17+ messages in thread
From: Andrii Nakryiko @ 2019-10-31 17:27 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

On Tue, Oct 29, 2019 at 12:39 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The existing pin_*() functions all try to create the parent directory
> before pinning. Move this check into the per-object _pin() functions
> instead. This ensures consistent behaviour when auto-pinning is
> added (which doesn't go through the top-level pin_maps() function), at the
> cost of a few more calls to mkdir().
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Makes sense, thanks.

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

>  tools/lib/bpf/libbpf.c |   61 +++++++++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map
  2019-10-31 17:26     ` Toke Høiland-Jørgensen
@ 2019-10-31 17:28       ` Andrii Nakryiko
  0 siblings, 0 replies; 17+ messages in thread
From: Andrii Nakryiko @ 2019-10-31 17:28 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

On Thu, Oct 31, 2019 at 10:26 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Tue, Oct 29, 2019 at 12:39 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).
> >>
> >> The behaviour of bpf_object__{un,}pin_maps() is changed so that if it is
> >> called with a NULL path argument (which was previously illegal), it will
> >> (un)pin only those maps that have a pin_path set.
> >>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >
> > Looks good, thanks! Just some minor things to fix up below.
> >
> > Acked-by: Andrii Nakryiko <andriin@fb.com>
> >
> >>  tools/lib/bpf/libbpf.c   |  164 +++++++++++++++++++++++++++++++++++-----------
> >>  tools/lib/bpf/libbpf.h   |    8 ++
> >>  tools/lib/bpf/libbpf.map |    3 +
> >>  3 files changed, 134 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index ce5ef3ddd263..fd11f6aeb32c 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,119 @@ 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->pin_path) {
> >> +               if (path && strcmp(path, map->pin_path)) {
> >> +                       pr_warn("map '%s' already has pin path '%s' different from '%s'\n",
> >> +                               bpf_map__name(map), map->pin_path, path);
> >> +                       return -EINVAL;
> >> +               } else if (map->pinned) {
> >> +                       pr_debug("map '%s' already pinned at '%s'; not re-pinning\n",
> >> +                                bpf_map__name(map), map->pin_path);
> >> +                       return 0;
> >> +               }
> >
> > `if (map->pinned)` check is the same in both branches, so I'd do it
> > first, before this map->pin_path if/else.
>
> But it's not. It's debug & return if pin_path is set, and an error
> otherwise.

Ah, right, it did feel weird to duplicate like that :) Ok, never mind then.

>
> Will fix the rest of your nits :)
>
> -Toke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map
  2019-10-31 17:22   ` Andrii Nakryiko
  2019-10-31 17:26     ` Toke Høiland-Jørgensen
@ 2019-10-31 17:31     ` Toke Høiland-Jørgensen
  2019-10-31 17:43       ` Andrii Nakryiko
  1 sibling, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-31 17:31 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

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

> [...]
>
>>
>>         return err;
>> @@ -4131,17 +4205,24 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
>>                 return -ENOENT;
>>
>>         bpf_object__for_each_map(map, obj) {
>> +               char *pin_path = NULL;
>>                 char buf[PATH_MAX];
>
> you can call buf as pin_path and get rid of extra pointer?

The idea here is to end up with bpf_map__unpin(map, NULL) if path is
unset. GCC complains if I reassign a static array pointer, so don't
think I can actually get rid of this?

-Toke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects
  2019-10-29 19:39 ` [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects Toke Høiland-Jørgensen
@ 2019-10-31 17:37   ` Andrii Nakryiko
  2019-10-31 17:52     ` Andrii Nakryiko
  0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2019-10-31 17:37 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

On Tue, Oct 29, 2019 at 12:39 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>
> ---

Please fix unconditional pin_path setting, with that:

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

>  tools/lib/bpf/bpf_helpers.h |    6 ++
>  tools/lib/bpf/libbpf.c      |  144 +++++++++++++++++++++++++++++++++++++++++--
>  tools/lib/bpf/libbpf.h      |   13 ++++
>  3 files changed, 154 insertions(+), 9 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;
>  };
>

[...]

> @@ -1270,6 +1292,28 @@ 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;
> +                       int err;
> +
> +                       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;
> +                       }
> +                       err = build_map_pin_path(map, pin_root_path);

uhm... only if (val == LIBBPF_PIN_BY_NAME)?.. maybe extend tests with
a mix if auto-pinned and never pinned map to catch issue like this?

> +                       if (err) {
> +                               pr_warn("map '%s': couldn't build pin path.\n",
> +                                       map_name);
> +                               return err;
> +                       }
>                 } else {
>                         if (strict) {
>                                 pr_warn("map '%s': unknown field '%s'.\n",
> @@ -1289,7 +1333,8 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
>         return 0;
>  }
>

[...]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map
  2019-10-31 17:31     ` Toke Høiland-Jørgensen
@ 2019-10-31 17:43       ` Andrii Nakryiko
  0 siblings, 0 replies; 17+ messages in thread
From: Andrii Nakryiko @ 2019-10-31 17:43 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

On Thu, Oct 31, 2019 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > [...]
> >
> >>
> >>         return err;
> >> @@ -4131,17 +4205,24 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
> >>                 return -ENOENT;
> >>
> >>         bpf_object__for_each_map(map, obj) {
> >> +               char *pin_path = NULL;
> >>                 char buf[PATH_MAX];
> >
> > you can call buf as pin_path and get rid of extra pointer?
>
> The idea here is to end up with bpf_map__unpin(map, NULL) if path is
> unset. GCC complains if I reassign a static array pointer, so don't
> think I can actually get rid of this?

oh, right, it's nullable pointer, then you do need buf and pin_path,
never mind. I keep forgetting this NULL semantics for pin/unpin :)

>
> -Toke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects
  2019-10-31 17:37   ` Andrii Nakryiko
@ 2019-10-31 17:52     ` Andrii Nakryiko
  2019-10-31 18:06       ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2019-10-31 17:52 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

On Thu, Oct 31, 2019 at 10:37 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 29, 2019 at 12:39 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>
> > ---
>
> Please fix unconditional pin_path setting, with that:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> >  tools/lib/bpf/bpf_helpers.h |    6 ++
> >  tools/lib/bpf/libbpf.c      |  144 +++++++++++++++++++++++++++++++++++++++++--
> >  tools/lib/bpf/libbpf.h      |   13 ++++
> >  3 files changed, 154 insertions(+), 9 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;
> >  };
> >
>
> [...]
>
> > @@ -1270,6 +1292,28 @@ 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;
> > +                       int err;
> > +
> > +                       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;
> > +                       }
> > +                       err = build_map_pin_path(map, pin_root_path);
>
> uhm... only if (val == LIBBPF_PIN_BY_NAME)?.. maybe extend tests with
> a mix if auto-pinned and never pinned map to catch issue like this?

I was wondering why your selftest didn't catch this, got puzzled for a
bit. It's because this code path will be executed only when map
defintion has __uint(pinning, LIBBPF_PIN_NONE), can you please add
that to selftest as well?

>
> > +                       if (err) {
> > +                               pr_warn("map '%s': couldn't build pin path.\n",
> > +                                       map_name);
> > +                               return err;
> > +                       }
> >                 } else {
> >                         if (strict) {
> >                                 pr_warn("map '%s': unknown field '%s'.\n",
> > @@ -1289,7 +1333,8 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> >         return 0;
> >  }
> >
>
> [...]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 5/5] selftests: Add tests for automatic map pinning
  2019-10-29 19:39 ` [PATCH bpf-next v4 5/5] selftests: Add tests for automatic map pinning Toke Høiland-Jørgensen
@ 2019-10-31 18:02   ` Andrii Nakryiko
  2019-10-31 18:18     ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2019-10-31 18:02 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

On Tue, Oct 29, 2019 at 12:39 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 |  157 ++++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/test_pinning.c |   29 ++++
>  2 files changed, 186 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..71f7dc51edc7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
> @@ -0,0 +1,157 @@
> +// 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"))
> +               return 0;
> +
> +       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;
> +}
> +
> +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";

Should this test mount/unmount (if necessary) /sys/fs/bpf? They will
all fail if BPF FS is not mounted, right?

> +       const char *pinpath = "/sys/fs/bpf/pinmap";
> +       const char *file = "./test_pinning.o";
> +       struct stat statbuf = {};
> +       struct bpf_object *obj;
> +       struct bpf_map *map;
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> +               .pin_root_path = custpath,
> +       );
> +
> +       int err;
> +       obj = bpf_object__open_file(file, NULL);
> +       if (CHECK_FAIL(libbpf_get_error(obj)))
> +               return;
> +
> +       err = bpf_object__load(obj);
> +       if (CHECK(err, "default load", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       /* check that pinmap was pinned */
> +       err = stat(pinpath, &statbuf);
> +       if (CHECK(err, "stat pinpath", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +        /* check that nopinmap was *not* pinned */
> +       err = stat(nopinpath, &statbuf);
> +       if (CHECK(!err || errno != ENOENT, "stat nopinpath",
> +                 "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +        map_id = get_map_id(obj, "pinmap");

something wrong with whitespaces here? can you please run
scripts/checkpatch.pl to double-check?

> +       if (!map_id)
> +               goto out;
> +
> +       bpf_object__close(obj);
> +
> +       obj = bpf_object__open_file(file, NULL);
> +       if (CHECK_FAIL(libbpf_get_error(obj)))

obj = NULL here before you go to out

> +               goto out;
> +
> +       err = bpf_object__load(obj);
> +       if (CHECK(err, "default load", "err %d errno %d\n", err, errno))
> +               goto out;
> +

[...]

> +       err = rmdir(custpath);
> +       if (CHECK(err, "rmdir custpindir", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       bpf_object__close(obj);
> +
> +       /* test auto-pinning at custom path with open opt */
> +       obj = bpf_object__open_file(file, &opts);
> +       if (CHECK_FAIL(libbpf_get_error(obj)))
> +               return;

obj = NULL; goto out; to ensure pinpath is unlinked?

> +
> +       err = bpf_object__load(obj);
> +       if (CHECK(err, "custom load", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       /* check that pinmap was pinned at the custom path */
> +       err = stat(custpinpath, &statbuf);
> +       if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +out:
> +       unlink(pinpath);
> +       unlink(nopinpath);
> +       unlink(custpinpath);
> +       rmdir(custpath);
> +       if (obj)
> +               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");


would be nice to ensure that __uint(pinning, LIBBPF_PIN_NONE) also
works as expected, do you mind adding one extra map?

> +
> +SEC("xdp_prog")
> +int _xdp_prog(struct xdp_md *xdp)
> +{
> +       return XDP_PASS;
> +}
> +
> +char _license[] SEC("license") = "GPL";
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects
  2019-10-31 17:52     ` Andrii Nakryiko
@ 2019-10-31 18:06       ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-31 18:06 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

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

> On Thu, Oct 31, 2019 at 10:37 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Tue, Oct 29, 2019 at 12:39 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>
>> > ---
>>
>> Please fix unconditional pin_path setting, with that:
>>
>> Acked-by: Andrii Nakryiko <andriin@fb.com>
>>
>> >  tools/lib/bpf/bpf_helpers.h |    6 ++
>> >  tools/lib/bpf/libbpf.c      |  144 +++++++++++++++++++++++++++++++++++++++++--
>> >  tools/lib/bpf/libbpf.h      |   13 ++++
>> >  3 files changed, 154 insertions(+), 9 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;
>> >  };
>> >
>>
>> [...]
>>
>> > @@ -1270,6 +1292,28 @@ 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;
>> > +                       int err;
>> > +
>> > +                       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;
>> > +                       }
>> > +                       err = build_map_pin_path(map, pin_root_path);
>>
>> uhm... only if (val == LIBBPF_PIN_BY_NAME)?.. maybe extend tests with
>> a mix if auto-pinned and never pinned map to catch issue like this?
>
> I was wondering why your selftest didn't catch this, got puzzled for a
> bit. It's because this code path will be executed only when map
> defintion has __uint(pinning, LIBBPF_PIN_NONE), can you please add
> that to selftest as well?

Can do :)

-Toke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH bpf-next v4 5/5] selftests: Add tests for automatic map pinning
  2019-10-31 18:02   ` Andrii Nakryiko
@ 2019-10-31 18:18     ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-31 18:18 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking,
	bpf

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

> On Tue, Oct 29, 2019 at 12:39 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 |  157 ++++++++++++++++++++++
>>  tools/testing/selftests/bpf/progs/test_pinning.c |   29 ++++
>>  2 files changed, 186 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..71f7dc51edc7
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
>> @@ -0,0 +1,157 @@
>> +// 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"))
>> +               return 0;
>> +
>> +       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;
>> +}
>> +
>> +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";
>
> Should this test mount/unmount (if necessary) /sys/fs/bpf? They will
> all fail if BPF FS is not mounted, right?

Yeah; I was kinda expecting that the test harness takes care of this. Is
it really a good idea for a selftest to mess with mount()?

>> +       const char *pinpath = "/sys/fs/bpf/pinmap";
>> +       const char *file = "./test_pinning.o";
>> +       struct stat statbuf = {};
>> +       struct bpf_object *obj;
>> +       struct bpf_map *map;
>> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
>> +               .pin_root_path = custpath,
>> +       );
>> +
>> +       int err;
>> +       obj = bpf_object__open_file(file, NULL);
>> +       if (CHECK_FAIL(libbpf_get_error(obj)))
>> +               return;
>> +
>> +       err = bpf_object__load(obj);
>> +       if (CHECK(err, "default load", "err %d errno %d\n", err, errno))
>> +               goto out;
>> +
>> +       /* check that pinmap was pinned */
>> +       err = stat(pinpath, &statbuf);
>> +       if (CHECK(err, "stat pinpath", "err %d errno %d\n", err, errno))
>> +               goto out;
>> +
>> +        /* check that nopinmap was *not* pinned */
>> +       err = stat(nopinpath, &statbuf);
>> +       if (CHECK(!err || errno != ENOENT, "stat nopinpath",
>> +                 "err %d errno %d\n", err, errno))
>> +               goto out;
>> +
>> +        map_id = get_map_id(obj, "pinmap");
>
> something wrong with whitespaces here? can you please run
> scripts/checkpatch.pl to double-check?

Yup, some space got in where a tab should be

>> +       if (!map_id)
>> +               goto out;
>> +
>> +       bpf_object__close(obj);
>> +
>> +       obj = bpf_object__open_file(file, NULL);
>> +       if (CHECK_FAIL(libbpf_get_error(obj)))
>
> obj = NULL here before you go to out

Yup

>
>> +               goto out;
>> +
>> +       err = bpf_object__load(obj);
>> +       if (CHECK(err, "default load", "err %d errno %d\n", err, errno))
>> +               goto out;
>> +
>
> [...]
>
>> +       err = rmdir(custpath);
>> +       if (CHECK(err, "rmdir custpindir", "err %d errno %d\n", err, errno))
>> +               goto out;
>> +
>> +       bpf_object__close(obj);
>> +
>> +       /* test auto-pinning at custom path with open opt */
>> +       obj = bpf_object__open_file(file, &opts);
>> +       if (CHECK_FAIL(libbpf_get_error(obj)))
>> +               return;
>
> obj = NULL; goto out; to ensure pinpath is unlinked?

Yeah.

>> +
>> +       err = bpf_object__load(obj);
>> +       if (CHECK(err, "custom load", "err %d errno %d\n", err, errno))
>> +               goto out;
>> +
>> +       /* check that pinmap was pinned at the custom path */
>> +       err = stat(custpinpath, &statbuf);
>> +       if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
>> +               goto out;
>> +
>> +out:
>> +       unlink(pinpath);
>> +       unlink(nopinpath);
>> +       unlink(custpinpath);
>> +       rmdir(custpath);
>> +       if (obj)
>> +               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");
>
>
> would be nice to ensure that __uint(pinning, LIBBPF_PIN_NONE) also
> works as expected, do you mind adding one extra map?

Sure, can do...

>> +
>> +SEC("xdp_prog")
>> +int _xdp_prog(struct xdp_md *xdp)
>> +{
>> +       return XDP_PASS;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-10-31 18:18 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-29 19:39 [PATCH bpf-next v4 0/5] libbpf: Support automatic pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
2019-10-29 19:39 ` [PATCH bpf-next v4 1/5] libbpf: Fix error handling in bpf_map__reuse_fd() Toke Høiland-Jørgensen
2019-10-29 19:39 ` [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map Toke Høiland-Jørgensen
2019-10-31 17:22   ` Andrii Nakryiko
2019-10-31 17:26     ` Toke Høiland-Jørgensen
2019-10-31 17:28       ` Andrii Nakryiko
2019-10-31 17:31     ` Toke Høiland-Jørgensen
2019-10-31 17:43       ` Andrii Nakryiko
2019-10-29 19:39 ` [PATCH bpf-next v4 3/5] libbpf: Move directory creation into _pin() functions Toke Høiland-Jørgensen
2019-10-31 17:27   ` Andrii Nakryiko
2019-10-29 19:39 ` [PATCH bpf-next v4 4/5] libbpf: Add auto-pinning of maps when loading BPF objects Toke Høiland-Jørgensen
2019-10-31 17:37   ` Andrii Nakryiko
2019-10-31 17:52     ` Andrii Nakryiko
2019-10-31 18:06       ` Toke Høiland-Jørgensen
2019-10-29 19:39 ` [PATCH bpf-next v4 5/5] selftests: Add tests for automatic map pinning Toke Høiland-Jørgensen
2019-10-31 18:02   ` Andrii Nakryiko
2019-10-31 18:18     ` Toke Høiland-Jørgensen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).