netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/5] bpftool: Switch to libbpf's hashmap for referencing BPF objects
@ 2021-10-22 17:16 Quentin Monnet
  2021-10-22 17:16 ` [PATCH bpf-next 1/5] bpftool: Remove Makefile dep. on $(LIBBPF) for $(LIBBPF_INTERNAL_HDRS) Quentin Monnet
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Quentin Monnet @ 2021-10-22 17:16 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

When listing BPF objects, bpftool can print a number of properties about
items holding references to these objects. For example, it can show pinned
paths for BPF programs, maps, and links; or programs and maps using a given
BTF object; or the names and PIDs of processes referencing BPF objects. To
collect this information, bpftool uses hash maps (to be clear: the data
structures, inside bpftool - we are not talking of BPF maps). It uses the
implementation available from the kernel, and picks it up from
tools/include/linux/hashtable.h.

This patchset converts bpftool's hash maps to a distinct implementation
instead, the one coming with libbpf. The main motivation for this change is
that it should ease the path towards a potential out-of-tree mirror for
bpftool, like the one libbpf already has. Although it's not perfect to
depend on libbpf's internal components, bpftool is intimately tied with the
library anyway, and this looks better than depending too much on (non-UAPI)
kernel headers.

The first two patches contain preparatory work on the Makefile and on the
initialisation of the hash maps for collecting pinned paths for objects.
Then the transition is split into several steps, one for each kind of
properties for which the collection is backed by hash maps.

Quentin Monnet (5):
  bpftool: Remove Makefile dep. on $(LIBBPF) for $(LIBBPF_INTERNAL_HDRS)
  bpftool: Do not expose and init hash maps for pinned path in main.c
  bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects
  bpftool: Switch to libbpf's hashmap for programs/maps in BTF listing
  bpftool: Switch to libbpf's hashmap for PIDs/names references

 tools/bpf/bpftool/Makefile |  12 ++--
 tools/bpf/bpftool/btf.c    | 133 +++++++++++++++----------------------
 tools/bpf/bpftool/common.c |  50 +++++++-------
 tools/bpf/bpftool/link.c   |  44 +++++++-----
 tools/bpf/bpftool/main.c   |  17 +----
 tools/bpf/bpftool/main.h   |  54 +++++++--------
 tools/bpf/bpftool/map.c    |  44 +++++++-----
 tools/bpf/bpftool/pids.c   |  84 ++++++++++++-----------
 tools/bpf/bpftool/prog.c   |  44 +++++++-----
 9 files changed, 246 insertions(+), 236 deletions(-)

-- 
2.30.2


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

* [PATCH bpf-next 1/5] bpftool: Remove Makefile dep. on $(LIBBPF) for $(LIBBPF_INTERNAL_HDRS)
  2021-10-22 17:16 [PATCH bpf-next 0/5] bpftool: Switch to libbpf's hashmap for referencing BPF objects Quentin Monnet
@ 2021-10-22 17:16 ` Quentin Monnet
  2021-10-22 17:16 ` [PATCH bpf-next 2/5] bpftool: Do not expose and init hash maps for pinned path in main.c Quentin Monnet
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Quentin Monnet @ 2021-10-22 17:16 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

The dependency is only useful to make sure that the $(LIBBPF_HDRS_DIR)
directory is created before we try to install locally the required
libbpf internal header. Let's create this directory properly instead.

This is in preparation of making $(LIBBPF_INTERNAL_HDRS) a dependency to
the bootstrap bpftool version, in which case we want no dependency on
$(LIBBPF).

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 098d762e111a..939b0fca5fb9 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -39,14 +39,14 @@ ifeq ($(BPFTOOL_VERSION),)
 BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
 endif
 
-$(LIBBPF_OUTPUT) $(BOOTSTRAP_OUTPUT) $(LIBBPF_BOOTSTRAP_OUTPUT):
+$(LIBBPF_OUTPUT) $(BOOTSTRAP_OUTPUT) $(LIBBPF_BOOTSTRAP_OUTPUT) $(LIBBPF_HDRS_DIR):
 	$(QUIET_MKDIR)mkdir -p $@
 
 $(LIBBPF): $(wildcard $(BPF_DIR)/*.[ch] $(BPF_DIR)/Makefile) | $(LIBBPF_OUTPUT)
 	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) \
 		DESTDIR=$(LIBBPF_DESTDIR) prefix= $(LIBBPF) install_headers
 
-$(LIBBPF_INTERNAL_HDRS): $(LIBBPF_HDRS_DIR)/%.h: $(BPF_DIR)/%.h $(LIBBPF)
+$(LIBBPF_INTERNAL_HDRS): $(LIBBPF_HDRS_DIR)/%.h: $(BPF_DIR)/%.h | $(LIBBPF_HDRS_DIR)
 	$(call QUIET_INSTALL, $@)
 	$(Q)install -m 644 -t $(LIBBPF_HDRS_DIR) $<
 
-- 
2.30.2


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

* [PATCH bpf-next 2/5] bpftool: Do not expose and init hash maps for pinned path in main.c
  2021-10-22 17:16 [PATCH bpf-next 0/5] bpftool: Switch to libbpf's hashmap for referencing BPF objects Quentin Monnet
  2021-10-22 17:16 ` [PATCH bpf-next 1/5] bpftool: Remove Makefile dep. on $(LIBBPF) for $(LIBBPF_INTERNAL_HDRS) Quentin Monnet
@ 2021-10-22 17:16 ` Quentin Monnet
  2021-10-23  0:13   ` Andrii Nakryiko
  2021-10-22 17:16 ` [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects Quentin Monnet
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Quentin Monnet @ 2021-10-22 17:16 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

BPF programs, maps, and links, can all be listed with their pinned paths
by bpftool, when the "-f" option is provided. To do so, bpftool builds
hash maps containing all pinned paths for each kind of objects.

These three hash maps are always initialised in main.c, and exposed
through main.h. There appear to be no particular reason to do so: we can
just as well make them static to the files that need them (prog.c,
map.c, and link.c respectively), and initialise them only when we want
to show objects and the "-f" switch is provided.

This may prevent unnecessary memory allocations if the implementation of
the hash maps was to change in the future.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/link.c |  9 ++++++++-
 tools/bpf/bpftool/main.c | 12 ------------
 tools/bpf/bpftool/main.h |  3 ---
 tools/bpf/bpftool/map.c  |  9 ++++++++-
 tools/bpf/bpftool/prog.c |  9 ++++++++-
 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index 8cc3e36f8cc6..ebf29be747b3 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -20,6 +20,8 @@ static const char * const link_type_name[] = {
 	[BPF_LINK_TYPE_NETNS]			= "netns",
 };
 
+static struct pinned_obj_table link_table;
+
 static int link_parse_fd(int *argc, char ***argv)
 {
 	int fd;
@@ -302,8 +304,10 @@ static int do_show(int argc, char **argv)
 	__u32 id = 0;
 	int err, fd;
 
-	if (show_pinned)
+	if (show_pinned) {
+		hash_init(link_table.table);
 		build_pinned_obj_table(&link_table, BPF_OBJ_LINK);
+	}
 	build_obj_refs_table(&refs_table, BPF_OBJ_LINK);
 
 	if (argc == 2) {
@@ -384,6 +388,9 @@ static int do_detach(int argc, char **argv)
 	if (json_output)
 		jsonw_null(json_wtr);
 
+	if (show_pinned)
+		delete_pinned_obj_table(&link_table);
+
 	return 0;
 }
 
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 02eaaf065f65..7a33f0e6da28 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -31,9 +31,6 @@ bool verifier_logs;
 bool relaxed_maps;
 bool use_loader;
 struct btf *base_btf;
-struct pinned_obj_table prog_table;
-struct pinned_obj_table map_table;
-struct pinned_obj_table link_table;
 struct obj_refs_table refs_table;
 
 static void __noreturn clean_and_exit(int i)
@@ -409,10 +406,6 @@ int main(int argc, char **argv)
 	block_mount = false;
 	bin_name = argv[0];
 
-	hash_init(prog_table.table);
-	hash_init(map_table.table);
-	hash_init(link_table.table);
-
 	opterr = 0;
 	while ((opt = getopt_long(argc, argv, "VhpjfLmndB:",
 				  options, NULL)) >= 0) {
@@ -479,11 +472,6 @@ int main(int argc, char **argv)
 	if (json_output)
 		jsonw_destroy(&json_wtr);
 
-	if (show_pinned) {
-		delete_pinned_obj_table(&prog_table);
-		delete_pinned_obj_table(&map_table);
-		delete_pinned_obj_table(&link_table);
-	}
 	btf__free(base_btf);
 
 	return ret;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 90caa42aac4c..baf607cd5924 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -91,9 +91,6 @@ extern bool verifier_logs;
 extern bool relaxed_maps;
 extern bool use_loader;
 extern struct btf *base_btf;
-extern struct pinned_obj_table prog_table;
-extern struct pinned_obj_table map_table;
-extern struct pinned_obj_table link_table;
 extern struct obj_refs_table refs_table;
 
 void __printf(1, 2) p_err(const char *fmt, ...);
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 407071d54ab1..0085039d9610 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -56,6 +56,8 @@ const char * const map_type_name[] = {
 
 const size_t map_type_name_size = ARRAY_SIZE(map_type_name);
 
+static struct pinned_obj_table map_table;
+
 static bool map_is_per_cpu(__u32 type)
 {
 	return type == BPF_MAP_TYPE_PERCPU_HASH ||
@@ -694,8 +696,10 @@ static int do_show(int argc, char **argv)
 	int err;
 	int fd;
 
-	if (show_pinned)
+	if (show_pinned) {
+		hash_init(map_table.table);
 		build_pinned_obj_table(&map_table, BPF_OBJ_MAP);
+	}
 	build_obj_refs_table(&refs_table, BPF_OBJ_MAP);
 
 	if (argc == 2)
@@ -742,6 +746,9 @@ static int do_show(int argc, char **argv)
 
 	delete_obj_refs_table(&refs_table);
 
+	if (show_pinned)
+		delete_pinned_obj_table(&map_table);
+
 	return errno == ENOENT ? 0 : -1;
 }
 
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 277d51c4c5d9..9a10cfebd252 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -84,6 +84,8 @@ static const char * const attach_type_strings[] = {
 	[__MAX_BPF_ATTACH_TYPE] = NULL,
 };
 
+static struct pinned_obj_table prog_table;
+
 static enum bpf_attach_type parse_attach_type(const char *str)
 {
 	enum bpf_attach_type type;
@@ -565,8 +567,10 @@ static int do_show(int argc, char **argv)
 	int err;
 	int fd;
 
-	if (show_pinned)
+	if (show_pinned) {
+		hash_init(prog_table.table);
 		build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
+	}
 	build_obj_refs_table(&refs_table, BPF_OBJ_PROG);
 
 	if (argc == 2)
@@ -611,6 +615,9 @@ static int do_show(int argc, char **argv)
 
 	delete_obj_refs_table(&refs_table);
 
+	if (show_pinned)
+		delete_pinned_obj_table(&prog_table);
+
 	return err;
 }
 
-- 
2.30.2


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

* [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects
  2021-10-22 17:16 [PATCH bpf-next 0/5] bpftool: Switch to libbpf's hashmap for referencing BPF objects Quentin Monnet
  2021-10-22 17:16 ` [PATCH bpf-next 1/5] bpftool: Remove Makefile dep. on $(LIBBPF) for $(LIBBPF_INTERNAL_HDRS) Quentin Monnet
  2021-10-22 17:16 ` [PATCH bpf-next 2/5] bpftool: Do not expose and init hash maps for pinned path in main.c Quentin Monnet
@ 2021-10-22 17:16 ` Quentin Monnet
  2021-10-23  0:54   ` Andrii Nakryiko
  2021-10-22 17:16 ` [PATCH bpf-next 4/5] bpftool: Switch to libbpf's hashmap for programs/maps in BTF listing Quentin Monnet
  2021-10-22 17:16 ` [PATCH bpf-next 5/5] bpftool: Switch to libbpf's hashmap for PIDs/names references Quentin Monnet
  4 siblings, 1 reply; 12+ messages in thread
From: Quentin Monnet @ 2021-10-22 17:16 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

In order to show pinned paths for BPF programs, maps, or links when
listing them with the "-f" option, bpftool creates hash maps to store
all relevant paths under the bpffs. So far, it would rely on the
kernel implementation (from tools/include/linux/hashtable.h).

We can make bpftool rely on libbpf's implementation instead. The
motivation is to make bpftool less dependent of kernel headers, to ease
the path to a potential out-of-tree mirror, like libbpf has.

This commit is the first step of the conversion: the hash maps for
pinned paths for programs, maps, and links are converted to libbpf's
hashmap.{c,h}. Other hash maps used for the PIDs of process holding
references to BPF objects are left unchanged for now. On the build side,
this requires adding a dependency to a second header internal to libbpf,
and making it a dependency for the bootstrap bpftool version as well.
The rest of the changes are a rather straightforward conversion.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/Makefile |  8 +++---
 tools/bpf/bpftool/common.c | 50 ++++++++++++++++++++------------------
 tools/bpf/bpftool/link.c   | 35 ++++++++++++++------------
 tools/bpf/bpftool/main.h   | 29 +++++++++++++---------
 tools/bpf/bpftool/map.c    | 35 ++++++++++++++------------
 tools/bpf/bpftool/prog.c   | 35 ++++++++++++++------------
 6 files changed, 105 insertions(+), 87 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 939b0fca5fb9..c0c30e56988f 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -31,9 +31,9 @@ LIBBPF = $(LIBBPF_OUTPUT)libbpf.a
 LIBBPF_BOOTSTRAP_OUTPUT = $(BOOTSTRAP_OUTPUT)libbpf/
 LIBBPF_BOOTSTRAP = $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
 
-# We need to copy nlattr.h which is not otherwise exported by libbpf, but still
-# required by bpftool.
-LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,nlattr.h)
+# We need to copy hashmap.h and nlattr.h which is not otherwise exported by
+# libbpf, but still required by bpftool.
+LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h)
 
 ifeq ($(BPFTOOL_VERSION),)
 BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
@@ -209,7 +209,7 @@ $(BPFTOOL_BOOTSTRAP): $(BOOTSTRAP_OBJS) $(LIBBPF_BOOTSTRAP)
 $(OUTPUT)bpftool: $(OBJS) $(LIBBPF)
 	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(OBJS) $(LIBS)
 
-$(BOOTSTRAP_OUTPUT)%.o: %.c | $(BOOTSTRAP_OUTPUT)
+$(BOOTSTRAP_OUTPUT)%.o: %.c $(LIBBPF_INTERNAL_HDRS) | $(BOOTSTRAP_OUTPUT)
 	$(QUIET_CC)$(HOSTCC) $(CFLAGS) -c -MMD -o $@ $<
 
 $(OUTPUT)%.o: %.c
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index d42d930a3ec4..c74b26e48831 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -22,6 +22,7 @@
 #include <sys/vfs.h>
 
 #include <bpf/bpf.h>
+#include <bpf/hashmap.h>
 #include <bpf/libbpf.h> /* libbpf_num_possible_cpus */
 
 #include "main.h"
@@ -393,7 +394,7 @@ void print_hex_data_json(uint8_t *data, size_t len)
 }
 
 /* extra params for nftw cb */
-static struct pinned_obj_table *build_fn_table;
+static struct hashmap *build_fn_table;
 static enum bpf_obj_type build_fn_type;
 
 static int do_build_table_cb(const char *fpath, const struct stat *sb,
@@ -401,9 +402,9 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb,
 {
 	struct bpf_prog_info pinned_info;
 	__u32 len = sizeof(pinned_info);
-	struct pinned_obj *obj_node;
 	enum bpf_obj_type objtype;
 	int fd, err = 0;
+	char *path;
 
 	if (typeflag != FTW_F)
 		goto out_ret;
@@ -420,28 +421,20 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb,
 	if (bpf_obj_get_info_by_fd(fd, &pinned_info, &len))
 		goto out_close;
 
-	obj_node = calloc(1, sizeof(*obj_node));
-	if (!obj_node) {
+	path = strdup(fpath);
+	if (!path) {
 		err = -1;
 		goto out_close;
 	}
 
-	obj_node->id = pinned_info.id;
-	obj_node->path = strdup(fpath);
-	if (!obj_node->path) {
-		err = -1;
-		free(obj_node);
-		goto out_close;
-	}
-
-	hash_add(build_fn_table->table, &obj_node->hash, obj_node->id);
+	hashmap__append(build_fn_table, u32_as_hash_field(pinned_info.id), path);
 out_close:
 	close(fd);
 out_ret:
 	return err;
 }
 
-int build_pinned_obj_table(struct pinned_obj_table *tab,
+int build_pinned_obj_table(struct hashmap *tab,
 			   enum bpf_obj_type type)
 {
 	struct mntent *mntent = NULL;
@@ -470,17 +463,18 @@ int build_pinned_obj_table(struct pinned_obj_table *tab,
 	return err;
 }
 
-void delete_pinned_obj_table(struct pinned_obj_table *tab)
+void delete_pinned_obj_table(struct hashmap *map)
 {
-	struct pinned_obj *obj;
-	struct hlist_node *tmp;
-	unsigned int bkt;
+	struct hashmap_entry *entry;
+	size_t bkt;
 
-	hash_for_each_safe(tab->table, bkt, tmp, obj, hash) {
-		hash_del(&obj->hash);
-		free(obj->path);
-		free(obj);
-	}
+	if (!map)
+		return;
+
+	hashmap__for_each_entry(map, entry, bkt)
+		free(entry->value);
+
+	hashmap__free(map);
 }
 
 unsigned int get_page_size(void)
@@ -962,3 +956,13 @@ int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len)
 
 	return fd;
 }
+
+size_t bpftool_hash_fn(const void *key, void *ctx)
+{
+	return (size_t)key;
+}
+
+bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx)
+{
+	return k1 == k2;
+}
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index ebf29be747b3..be802e6ccc53 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -7,6 +7,7 @@
 #include <unistd.h>
 
 #include <bpf/bpf.h>
+#include <bpf/hashmap.h>
 
 #include "json_writer.h"
 #include "main.h"
@@ -20,7 +21,7 @@ static const char * const link_type_name[] = {
 	[BPF_LINK_TYPE_NETNS]			= "netns",
 };
 
-static struct pinned_obj_table link_table;
+static struct hashmap *link_table;
 
 static int link_parse_fd(int *argc, char ***argv)
 {
@@ -158,15 +159,14 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
 		break;
 	}
 
-	if (!hash_empty(link_table.table)) {
-		struct pinned_obj *obj;
+	if (!hashmap__empty(link_table)) {
+		struct hashmap_entry *entry;
 
 		jsonw_name(json_wtr, "pinned");
 		jsonw_start_array(json_wtr);
-		hash_for_each_possible(link_table.table, obj, hash, info->id) {
-			if (obj->id == info->id)
-				jsonw_string(json_wtr, obj->path);
-		}
+		hashmap__for_each_key_entry(link_table, entry,
+					    u32_as_hash_field(info->id))
+			jsonw_string(json_wtr, entry->value);
 		jsonw_end_array(json_wtr);
 	}
 
@@ -246,13 +246,12 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
 		break;
 	}
 
-	if (!hash_empty(link_table.table)) {
-		struct pinned_obj *obj;
+	if (!hashmap__empty(link_table)) {
+		struct hashmap_entry *entry;
 
-		hash_for_each_possible(link_table.table, obj, hash, info->id) {
-			if (obj->id == info->id)
-				printf("\n\tpinned %s", obj->path);
-		}
+		hashmap__for_each_key_entry(link_table, entry,
+					    u32_as_hash_field(info->id))
+			printf("\n\tpinned %s", (char *)entry->value);
 	}
 	emit_obj_refs_plain(&refs_table, info->id, "\n\tpids ");
 
@@ -305,8 +304,12 @@ static int do_show(int argc, char **argv)
 	int err, fd;
 
 	if (show_pinned) {
-		hash_init(link_table.table);
-		build_pinned_obj_table(&link_table, BPF_OBJ_LINK);
+		link_table = hashmap__new(bpftool_hash_fn, bpftool_equal_fn, NULL);
+		if (!link_table) {
+			p_err("failed to create hashmap for pinned paths");
+			return -1;
+		}
+		build_pinned_obj_table(link_table, BPF_OBJ_LINK);
 	}
 	build_obj_refs_table(&refs_table, BPF_OBJ_LINK);
 
@@ -389,7 +392,7 @@ static int do_detach(int argc, char **argv)
 		jsonw_null(json_wtr);
 
 	if (show_pinned)
-		delete_pinned_obj_table(&link_table);
+		delete_pinned_obj_table(link_table);
 
 	return 0;
 }
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index baf607cd5924..f61be172d864 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -14,6 +14,7 @@
 #include <linux/hashtable.h>
 #include <tools/libc_compat.h>
 
+#include <bpf/hashmap.h>
 #include <bpf/libbpf.h>
 
 #include "json_writer.h"
@@ -105,16 +106,6 @@ void set_max_rlimit(void);
 
 int mount_tracefs(const char *target);
 
-struct pinned_obj_table {
-	DECLARE_HASHTABLE(table, 16);
-};
-
-struct pinned_obj {
-	__u32 id;
-	char *path;
-	struct hlist_node hash;
-};
-
 struct obj_refs_table {
 	DECLARE_HASHTABLE(table, 16);
 };
@@ -134,9 +125,9 @@ struct obj_refs {
 struct btf;
 struct bpf_line_info;
 
-int build_pinned_obj_table(struct pinned_obj_table *table,
+int build_pinned_obj_table(struct hashmap *table,
 			   enum bpf_obj_type type);
-void delete_pinned_obj_table(struct pinned_obj_table *tab);
+void delete_pinned_obj_table(struct hashmap *table);
 __weak int build_obj_refs_table(struct obj_refs_table *table,
 				enum bpf_obj_type type);
 __weak void delete_obj_refs_table(struct obj_refs_table *table);
@@ -256,4 +247,18 @@ int do_filter_dump(struct tcmsg *ifinfo, struct nlattr **tb, const char *kind,
 
 int print_all_levels(__maybe_unused enum libbpf_print_level level,
 		     const char *format, va_list args);
+
+size_t bpftool_hash_fn(const void *key, void *ctx);
+bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx);
+
+static inline void *u32_as_hash_field(__u32 x)
+{
+	return (void *)(uintptr_t)x;
+}
+
+static inline bool hashmap__empty(struct hashmap *map)
+{
+	return map ? hashmap__size(map) == 0 : true;
+}
+
 #endif
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 0085039d9610..a2c19324efa7 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -17,6 +17,7 @@
 
 #include <bpf/bpf.h>
 #include <bpf/btf.h>
+#include <bpf/hashmap.h>
 
 #include "json_writer.h"
 #include "main.h"
@@ -56,7 +57,7 @@ const char * const map_type_name[] = {
 
 const size_t map_type_name_size = ARRAY_SIZE(map_type_name);
 
-static struct pinned_obj_table map_table;
+static struct hashmap *map_table;
 
 static bool map_is_per_cpu(__u32 type)
 {
@@ -537,15 +538,14 @@ static int show_map_close_json(int fd, struct bpf_map_info *info)
 	if (info->btf_id)
 		jsonw_int_field(json_wtr, "btf_id", info->btf_id);
 
-	if (!hash_empty(map_table.table)) {
-		struct pinned_obj *obj;
+	if (!hashmap__empty(map_table)) {
+		struct hashmap_entry *entry;
 
 		jsonw_name(json_wtr, "pinned");
 		jsonw_start_array(json_wtr);
-		hash_for_each_possible(map_table.table, obj, hash, info->id) {
-			if (obj->id == info->id)
-				jsonw_string(json_wtr, obj->path);
-		}
+		hashmap__for_each_key_entry(map_table, entry,
+					    u32_as_hash_field(info->id))
+			jsonw_string(json_wtr, entry->value);
 		jsonw_end_array(json_wtr);
 	}
 
@@ -612,13 +612,12 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
 	}
 	close(fd);
 
-	if (!hash_empty(map_table.table)) {
-		struct pinned_obj *obj;
+	if (!hashmap__empty(map_table)) {
+		struct hashmap_entry *entry;
 
-		hash_for_each_possible(map_table.table, obj, hash, info->id) {
-			if (obj->id == info->id)
-				printf("\n\tpinned %s", obj->path);
-		}
+		hashmap__for_each_key_entry(map_table, entry,
+					    u32_as_hash_field(info->id))
+			printf("\n\tpinned %s", (char *)entry->value);
 	}
 	printf("\n");
 
@@ -697,8 +696,12 @@ static int do_show(int argc, char **argv)
 	int fd;
 
 	if (show_pinned) {
-		hash_init(map_table.table);
-		build_pinned_obj_table(&map_table, BPF_OBJ_MAP);
+		map_table = hashmap__new(bpftool_hash_fn, bpftool_equal_fn, NULL);
+		if (!map_table) {
+			p_err("failed to create hashmap for pinned paths");
+			return -1;
+		}
+		build_pinned_obj_table(map_table, BPF_OBJ_MAP);
 	}
 	build_obj_refs_table(&refs_table, BPF_OBJ_MAP);
 
@@ -747,7 +750,7 @@ static int do_show(int argc, char **argv)
 	delete_obj_refs_table(&refs_table);
 
 	if (show_pinned)
-		delete_pinned_obj_table(&map_table);
+		delete_pinned_obj_table(map_table);
 
 	return errno == ENOENT ? 0 : -1;
 }
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 9a10cfebd252..20026b4178b0 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -24,6 +24,7 @@
 
 #include <bpf/bpf.h>
 #include <bpf/btf.h>
+#include <bpf/hashmap.h>
 #include <bpf/libbpf.h>
 #include <bpf/skel_internal.h>
 
@@ -84,7 +85,7 @@ static const char * const attach_type_strings[] = {
 	[__MAX_BPF_ATTACH_TYPE] = NULL,
 };
 
-static struct pinned_obj_table prog_table;
+static struct hashmap *prog_table;
 
 static enum bpf_attach_type parse_attach_type(const char *str)
 {
@@ -416,15 +417,14 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
 	if (info->btf_id)
 		jsonw_int_field(json_wtr, "btf_id", info->btf_id);
 
-	if (!hash_empty(prog_table.table)) {
-		struct pinned_obj *obj;
+	if (!hashmap__empty(prog_table)) {
+		struct hashmap_entry *entry;
 
 		jsonw_name(json_wtr, "pinned");
 		jsonw_start_array(json_wtr);
-		hash_for_each_possible(prog_table.table, obj, hash, info->id) {
-			if (obj->id == info->id)
-				jsonw_string(json_wtr, obj->path);
-		}
+		hashmap__for_each_key_entry(prog_table, entry,
+					    u32_as_hash_field(info->id))
+			jsonw_string(json_wtr, entry->value);
 		jsonw_end_array(json_wtr);
 	}
 
@@ -488,13 +488,12 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
 	if (info->nr_map_ids)
 		show_prog_maps(fd, info->nr_map_ids);
 
-	if (!hash_empty(prog_table.table)) {
-		struct pinned_obj *obj;
+	if (!hashmap__empty(prog_table)) {
+		struct hashmap_entry *entry;
 
-		hash_for_each_possible(prog_table.table, obj, hash, info->id) {
-			if (obj->id == info->id)
-				printf("\n\tpinned %s", obj->path);
-		}
+		hashmap__for_each_key_entry(prog_table, entry,
+					    u32_as_hash_field(info->id))
+			printf("\n\tpinned %s", (char *)entry->value);
 	}
 
 	if (info->btf_id)
@@ -568,8 +567,12 @@ static int do_show(int argc, char **argv)
 	int fd;
 
 	if (show_pinned) {
-		hash_init(prog_table.table);
-		build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
+		prog_table = hashmap__new(bpftool_hash_fn, bpftool_equal_fn, NULL);
+		if (!prog_table) {
+			p_err("failed to create hashmap for pinned paths");
+			return -1;
+		}
+		build_pinned_obj_table(prog_table, BPF_OBJ_PROG);
 	}
 	build_obj_refs_table(&refs_table, BPF_OBJ_PROG);
 
@@ -616,7 +619,7 @@ static int do_show(int argc, char **argv)
 	delete_obj_refs_table(&refs_table);
 
 	if (show_pinned)
-		delete_pinned_obj_table(&prog_table);
+		delete_pinned_obj_table(prog_table);
 
 	return err;
 }
-- 
2.30.2


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

* [PATCH bpf-next 4/5] bpftool: Switch to libbpf's hashmap for programs/maps in BTF listing
  2021-10-22 17:16 [PATCH bpf-next 0/5] bpftool: Switch to libbpf's hashmap for referencing BPF objects Quentin Monnet
                   ` (2 preceding siblings ...)
  2021-10-22 17:16 ` [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects Quentin Monnet
@ 2021-10-22 17:16 ` Quentin Monnet
  2021-10-23  1:47   ` Andrii Nakryiko
  2021-10-22 17:16 ` [PATCH bpf-next 5/5] bpftool: Switch to libbpf's hashmap for PIDs/names references Quentin Monnet
  4 siblings, 1 reply; 12+ messages in thread
From: Quentin Monnet @ 2021-10-22 17:16 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

In order to show BPF programs and maps using BTF objects when the latter
are being listed, bpftool creates hash maps to store all relevant items.
This commit is part of a set that transitions from the kernel's hash map
implementation to the one coming with libbpf.

The motivation is to make bpftool less dependent of kernel headers, to
ease the path to a potential out-of-tree mirror, like libbpf has.

This commit focuses on the two hash maps used by bpftool when listing
BTF objects to store references to programs and maps, and convert them
to the libbpf's implementation.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/btf.c  | 126 ++++++++++++++++-----------------------
 tools/bpf/bpftool/main.h |   5 ++
 2 files changed, 57 insertions(+), 74 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 7b68d4f65fe6..84959aa05265 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -8,14 +8,16 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
-#include <bpf/bpf.h>
-#include <bpf/btf.h>
-#include <bpf/libbpf.h>
 #include <linux/btf.h>
 #include <linux/hashtable.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#include <bpf/bpf.h>
+#include <bpf/btf.h>
+#include <bpf/hashmap.h>
+#include <bpf/libbpf.h>
+
 #include "json_writer.h"
 #include "main.h"
 
@@ -40,14 +42,9 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
 	[BTF_KIND_DECL_TAG]	= "DECL_TAG",
 };
 
-struct btf_attach_table {
-	DECLARE_HASHTABLE(table, 16);
-};
-
 struct btf_attach_point {
 	__u32 obj_id;
 	__u32 btf_id;
-	struct hlist_node hash;
 };
 
 static const char *btf_int_enc_str(__u8 encoding)
@@ -645,21 +642,8 @@ static int btf_parse_fd(int *argc, char ***argv)
 	return fd;
 }
 
-static void delete_btf_table(struct btf_attach_table *tab)
-{
-	struct btf_attach_point *obj;
-	struct hlist_node *tmp;
-
-	unsigned int bkt;
-
-	hash_for_each_safe(tab->table, bkt, tmp, obj, hash) {
-		hash_del(&obj->hash);
-		free(obj);
-	}
-}
-
 static int
-build_btf_type_table(struct btf_attach_table *tab, enum bpf_obj_type type,
+build_btf_type_table(struct hashmap *tab, enum bpf_obj_type type,
 		     void *info, __u32 *len)
 {
 	static const char * const names[] = {
@@ -667,7 +651,6 @@ build_btf_type_table(struct btf_attach_table *tab, enum bpf_obj_type type,
 		[BPF_OBJ_PROG]		= "prog",
 		[BPF_OBJ_MAP]		= "map",
 	};
-	struct btf_attach_point *obj_node;
 	__u32 btf_id, id = 0;
 	int err;
 	int fd;
@@ -741,28 +724,20 @@ build_btf_type_table(struct btf_attach_table *tab, enum bpf_obj_type type,
 		if (!btf_id)
 			continue;
 
-		obj_node = calloc(1, sizeof(*obj_node));
-		if (!obj_node) {
-			p_err("failed to allocate memory: %s", strerror(errno));
-			err = -ENOMEM;
-			goto err_free;
-		}
-
-		obj_node->obj_id = id;
-		obj_node->btf_id = btf_id;
-		hash_add(tab->table, &obj_node->hash, obj_node->btf_id);
+		hashmap__append(tab, u32_as_hash_field(btf_id),
+				u32_as_hash_field(id));
 	}
 
 	return 0;
 
 err_free:
-	delete_btf_table(tab);
+	hashmap__free(tab);
 	return err;
 }
 
 static int
-build_btf_tables(struct btf_attach_table *btf_prog_table,
-		 struct btf_attach_table *btf_map_table)
+build_btf_tables(struct hashmap *btf_prog_table,
+		 struct hashmap *btf_map_table)
 {
 	struct bpf_prog_info prog_info;
 	__u32 prog_len = sizeof(prog_info);
@@ -778,7 +753,7 @@ build_btf_tables(struct btf_attach_table *btf_prog_table,
 	err = build_btf_type_table(btf_map_table, BPF_OBJ_MAP, &map_info,
 				   &map_len);
 	if (err) {
-		delete_btf_table(btf_prog_table);
+		hashmap__free(btf_prog_table);
 		return err;
 	}
 
@@ -787,10 +762,10 @@ build_btf_tables(struct btf_attach_table *btf_prog_table,
 
 static void
 show_btf_plain(struct bpf_btf_info *info, int fd,
-	       struct btf_attach_table *btf_prog_table,
-	       struct btf_attach_table *btf_map_table)
+	       struct hashmap *btf_prog_table,
+	       struct hashmap *btf_map_table)
 {
-	struct btf_attach_point *obj;
+	struct hashmap_entry *entry;
 	const char *name = u64_to_ptr(info->name);
 	int n;
 
@@ -804,18 +779,17 @@ show_btf_plain(struct bpf_btf_info *info, int fd,
 	printf("size %uB", info->btf_size);
 
 	n = 0;
-	hash_for_each_possible(btf_prog_table->table, obj, hash, info->id) {
-		if (obj->btf_id == info->id)
-			printf("%s%u", n++ == 0 ? "  prog_ids " : ",",
-			       obj->obj_id);
-	}
+	hashmap__for_each_key_entry(btf_prog_table, entry,
+				    u32_as_hash_field(info->id))
+		printf("%s%u", n++ == 0 ? "  prog_ids " : ",",
+		       hash_field_as_u32(entry->value));
 
 	n = 0;
-	hash_for_each_possible(btf_map_table->table, obj, hash, info->id) {
-		if (obj->btf_id == info->id)
-			printf("%s%u", n++ == 0 ? "  map_ids " : ",",
-			       obj->obj_id);
-	}
+	hashmap__for_each_key_entry(btf_map_table, entry,
+				    u32_as_hash_field(info->id))
+		printf("%s%u", n++ == 0 ? "  map_ids " : ",",
+		       hash_field_as_u32(entry->value));
+
 	emit_obj_refs_plain(&refs_table, info->id, "\n\tpids ");
 
 	printf("\n");
@@ -823,10 +797,10 @@ show_btf_plain(struct bpf_btf_info *info, int fd,
 
 static void
 show_btf_json(struct bpf_btf_info *info, int fd,
-	      struct btf_attach_table *btf_prog_table,
-	      struct btf_attach_table *btf_map_table)
+	      struct hashmap *btf_prog_table,
+	      struct hashmap *btf_map_table)
 {
-	struct btf_attach_point *obj;
+	struct hashmap_entry *entry;
 	const char *name = u64_to_ptr(info->name);
 
 	jsonw_start_object(json_wtr);	/* btf object */
@@ -835,20 +809,16 @@ show_btf_json(struct bpf_btf_info *info, int fd,
 
 	jsonw_name(json_wtr, "prog_ids");
 	jsonw_start_array(json_wtr);	/* prog_ids */
-	hash_for_each_possible(btf_prog_table->table, obj, hash,
-			       info->id) {
-		if (obj->btf_id == info->id)
-			jsonw_uint(json_wtr, obj->obj_id);
-	}
+	hashmap__for_each_key_entry(btf_prog_table, entry,
+				    u32_as_hash_field(info->id))
+		jsonw_uint(json_wtr, hash_field_as_u32(entry->value));
 	jsonw_end_array(json_wtr);	/* prog_ids */
 
 	jsonw_name(json_wtr, "map_ids");
 	jsonw_start_array(json_wtr);	/* map_ids */
-	hash_for_each_possible(btf_map_table->table, obj, hash,
-			       info->id) {
-		if (obj->btf_id == info->id)
-			jsonw_uint(json_wtr, obj->obj_id);
-	}
+	hashmap__for_each_key_entry(btf_map_table, entry,
+				    u32_as_hash_field(info->id))
+		jsonw_uint(json_wtr, hash_field_as_u32(entry->value));
 	jsonw_end_array(json_wtr);	/* map_ids */
 
 	emit_obj_refs_json(&refs_table, info->id, json_wtr); /* pids */
@@ -862,8 +832,8 @@ show_btf_json(struct bpf_btf_info *info, int fd,
 }
 
 static int
-show_btf(int fd, struct btf_attach_table *btf_prog_table,
-	 struct btf_attach_table *btf_map_table)
+show_btf(int fd, struct hashmap *btf_prog_table,
+	 struct hashmap *btf_map_table)
 {
 	struct bpf_btf_info info;
 	__u32 len = sizeof(info);
@@ -900,8 +870,8 @@ show_btf(int fd, struct btf_attach_table *btf_prog_table,
 
 static int do_show(int argc, char **argv)
 {
-	struct btf_attach_table btf_prog_table;
-	struct btf_attach_table btf_map_table;
+	struct hashmap *btf_prog_table;
+	struct hashmap *btf_map_table;
 	int err, fd = -1;
 	__u32 id = 0;
 
@@ -917,9 +887,17 @@ static int do_show(int argc, char **argv)
 		return BAD_ARG();
 	}
 
-	hash_init(btf_prog_table.table);
-	hash_init(btf_map_table.table);
-	err = build_btf_tables(&btf_prog_table, &btf_map_table);
+	btf_prog_table = hashmap__new(bpftool_hash_fn, bpftool_equal_fn, NULL);
+	btf_map_table = hashmap__new(bpftool_hash_fn, bpftool_equal_fn, NULL);
+	if (!btf_prog_table || !btf_map_table) {
+		hashmap__free(btf_prog_table);
+		hashmap__free(btf_map_table);
+		if (fd >= 0)
+			close(fd);
+		p_err("failed to create hashmap for object references");
+		return -1;
+	}
+	err = build_btf_tables(btf_prog_table, btf_map_table);
 	if (err) {
 		if (fd >= 0)
 			close(fd);
@@ -928,7 +906,7 @@ static int do_show(int argc, char **argv)
 	build_obj_refs_table(&refs_table, BPF_OBJ_BTF);
 
 	if (fd >= 0) {
-		err = show_btf(fd, &btf_prog_table, &btf_map_table);
+		err = show_btf(fd, btf_prog_table, btf_map_table);
 		close(fd);
 		goto exit_free;
 	}
@@ -960,7 +938,7 @@ static int do_show(int argc, char **argv)
 			break;
 		}
 
-		err = show_btf(fd, &btf_prog_table, &btf_map_table);
+		err = show_btf(fd, btf_prog_table, btf_map_table);
 		close(fd);
 		if (err)
 			break;
@@ -970,8 +948,8 @@ static int do_show(int argc, char **argv)
 		jsonw_end_array(json_wtr);	/* root array */
 
 exit_free:
-	delete_btf_table(&btf_prog_table);
-	delete_btf_table(&btf_map_table);
+	hashmap__free(btf_prog_table);
+	hashmap__free(btf_map_table);
 	delete_obj_refs_table(&refs_table);
 
 	return err;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index f61be172d864..a8e71ead848c 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -256,6 +256,11 @@ static inline void *u32_as_hash_field(__u32 x)
 	return (void *)(uintptr_t)x;
 }
 
+static inline __u32 hash_field_as_u32(const void *x)
+{
+	return (__u32)(uintptr_t)x;
+}
+
 static inline bool hashmap__empty(struct hashmap *map)
 {
 	return map ? hashmap__size(map) == 0 : true;
-- 
2.30.2


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

* [PATCH bpf-next 5/5] bpftool: Switch to libbpf's hashmap for PIDs/names references
  2021-10-22 17:16 [PATCH bpf-next 0/5] bpftool: Switch to libbpf's hashmap for referencing BPF objects Quentin Monnet
                   ` (3 preceding siblings ...)
  2021-10-22 17:16 ` [PATCH bpf-next 4/5] bpftool: Switch to libbpf's hashmap for programs/maps in BTF listing Quentin Monnet
@ 2021-10-22 17:16 ` Quentin Monnet
  2021-10-23  1:50   ` Andrii Nakryiko
  4 siblings, 1 reply; 12+ messages in thread
From: Quentin Monnet @ 2021-10-22 17:16 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

In order to show PIDs and names for processes holding references to BPF
programs, maps, links, or BTF objects, bpftool creates hash maps to
store all relevant information. This commit is part of a set that
transitions from the kernel's hash map implementation to the one coming
with libbpf.

The motivation is to make bpftool less dependent of kernel headers, to
ease the path to a potential out-of-tree mirror, like libbpf has.

This is the third and final step of the transition, in which we convert
the hash maps used for storing the information about the processes
holding references to BPF objects (programs, maps, links, BTF), and at
last we drop the inclusion of tools/include/linux/hashtable.h.

Note: Checkpatch complains about the use of __weak declarations, and the
missing empty lines after the bunch of empty function declarations when
compiling without the BPF skeletons (none of these were introduced in
this patch). We want to keep things as they are, and the reports should
be safe to ignore.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/btf.c  |  7 ++--
 tools/bpf/bpftool/link.c |  6 +--
 tools/bpf/bpftool/main.c |  5 ++-
 tools/bpf/bpftool/main.h | 17 +++-----
 tools/bpf/bpftool/map.c  |  6 +--
 tools/bpf/bpftool/pids.c | 84 ++++++++++++++++++++++------------------
 tools/bpf/bpftool/prog.c |  6 +--
 7 files changed, 67 insertions(+), 64 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 84959aa05265..2a07b460d728 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -9,7 +9,6 @@
 #include <string.h>
 #include <unistd.h>
 #include <linux/btf.h>
-#include <linux/hashtable.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
@@ -790,7 +789,7 @@ show_btf_plain(struct bpf_btf_info *info, int fd,
 		printf("%s%u", n++ == 0 ? "  map_ids " : ",",
 		       hash_field_as_u32(entry->value));
 
-	emit_obj_refs_plain(&refs_table, info->id, "\n\tpids ");
+	emit_obj_refs_plain(refs_table, info->id, "\n\tpids ");
 
 	printf("\n");
 }
@@ -821,7 +820,7 @@ show_btf_json(struct bpf_btf_info *info, int fd,
 		jsonw_uint(json_wtr, hash_field_as_u32(entry->value));
 	jsonw_end_array(json_wtr);	/* map_ids */
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr); /* pids */
+	emit_obj_refs_json(refs_table, info->id, json_wtr); /* pids */
 
 	jsonw_bool_field(json_wtr, "kernel", info->kernel_btf);
 
@@ -950,7 +949,7 @@ static int do_show(int argc, char **argv)
 exit_free:
 	hashmap__free(btf_prog_table);
 	hashmap__free(btf_map_table);
-	delete_obj_refs_table(&refs_table);
+	delete_obj_refs_table(refs_table);
 
 	return err;
 }
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index be802e6ccc53..00cb8aea6eec 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -170,7 +170,7 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
 		jsonw_end_array(json_wtr);
 	}
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr);
+	emit_obj_refs_json(refs_table, info->id, json_wtr);
 
 	jsonw_end_object(json_wtr);
 
@@ -253,7 +253,7 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
 					    u32_as_hash_field(info->id))
 			printf("\n\tpinned %s", (char *)entry->value);
 	}
-	emit_obj_refs_plain(&refs_table, info->id, "\n\tpids ");
+	emit_obj_refs_plain(refs_table, info->id, "\n\tpids ");
 
 	printf("\n");
 
@@ -351,7 +351,7 @@ static int do_show(int argc, char **argv)
 	if (json_output)
 		jsonw_end_array(json_wtr);
 
-	delete_obj_refs_table(&refs_table);
+	delete_obj_refs_table(refs_table);
 
 	return errno == ENOENT ? 0 : -1;
 }
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 7a33f0e6da28..28237d7cef67 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -10,8 +10,9 @@
 #include <string.h>
 
 #include <bpf/bpf.h>
-#include <bpf/libbpf.h>
 #include <bpf/btf.h>
+#include <bpf/hashmap.h>
+#include <bpf/libbpf.h>
 
 #include "main.h"
 
@@ -31,7 +32,7 @@ bool verifier_logs;
 bool relaxed_maps;
 bool use_loader;
 struct btf *base_btf;
-struct obj_refs_table refs_table;
+struct hashmap *refs_table;
 
 static void __noreturn clean_and_exit(int i)
 {
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index a8e71ead848c..0344afd1dbbd 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -11,7 +11,6 @@
 #include <linux/bpf.h>
 #include <linux/compiler.h>
 #include <linux/kernel.h>
-#include <linux/hashtable.h>
 #include <tools/libc_compat.h>
 
 #include <bpf/hashmap.h>
@@ -92,7 +91,7 @@ extern bool verifier_logs;
 extern bool relaxed_maps;
 extern bool use_loader;
 extern struct btf *base_btf;
-extern struct obj_refs_table refs_table;
+extern struct hashmap *refs_table;
 
 void __printf(1, 2) p_err(const char *fmt, ...);
 void __printf(1, 2) p_info(const char *fmt, ...);
@@ -106,18 +105,12 @@ void set_max_rlimit(void);
 
 int mount_tracefs(const char *target);
 
-struct obj_refs_table {
-	DECLARE_HASHTABLE(table, 16);
-};
-
 struct obj_ref {
 	int pid;
 	char comm[16];
 };
 
 struct obj_refs {
-	struct hlist_node node;
-	__u32 id;
 	int ref_cnt;
 	struct obj_ref *refs;
 };
@@ -128,12 +121,12 @@ struct bpf_line_info;
 int build_pinned_obj_table(struct hashmap *table,
 			   enum bpf_obj_type type);
 void delete_pinned_obj_table(struct hashmap *table);
-__weak int build_obj_refs_table(struct obj_refs_table *table,
+__weak int build_obj_refs_table(struct hashmap **table,
 				enum bpf_obj_type type);
-__weak void delete_obj_refs_table(struct obj_refs_table *table);
-__weak void emit_obj_refs_json(struct obj_refs_table *table, __u32 id,
+__weak void delete_obj_refs_table(struct hashmap *table);
+__weak void emit_obj_refs_json(struct hashmap *table, __u32 id,
 			       json_writer_t *json_wtr);
-__weak void emit_obj_refs_plain(struct obj_refs_table *table, __u32 id,
+__weak void emit_obj_refs_plain(struct hashmap *table, __u32 id,
 				const char *prefix);
 void print_dev_plain(__u32 ifindex, __u64 ns_dev, __u64 ns_inode);
 void print_dev_json(__u32 ifindex, __u64 ns_dev, __u64 ns_inode);
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index a2c19324efa7..3479639664d0 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -549,7 +549,7 @@ static int show_map_close_json(int fd, struct bpf_map_info *info)
 		jsonw_end_array(json_wtr);
 	}
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr);
+	emit_obj_refs_json(refs_table, info->id, json_wtr);
 
 	jsonw_end_object(json_wtr);
 
@@ -637,7 +637,7 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
 	if (frozen)
 		printf("%sfrozen", info->btf_id ? "  " : "");
 
-	emit_obj_refs_plain(&refs_table, info->id, "\n\tpids ");
+	emit_obj_refs_plain(refs_table, info->id, "\n\tpids ");
 
 	printf("\n");
 	return 0;
@@ -747,7 +747,7 @@ static int do_show(int argc, char **argv)
 	if (json_output)
 		jsonw_end_array(json_wtr);
 
-	delete_obj_refs_table(&refs_table);
+	delete_obj_refs_table(refs_table);
 
 	if (show_pinned)
 		delete_pinned_obj_table(map_table);
diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
index 477e55d59c34..02fea61243c8 100644
--- a/tools/bpf/bpftool/pids.c
+++ b/tools/bpf/bpftool/pids.c
@@ -6,35 +6,37 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+
 #include <bpf/bpf.h>
+#include <bpf/hashmap.h>
 
 #include "main.h"
 #include "skeleton/pid_iter.h"
 
 #ifdef BPFTOOL_WITHOUT_SKELETONS
 
-int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type)
+int build_obj_refs_table(struct hashmap **map, enum bpf_obj_type type)
 {
 	return -ENOTSUP;
 }
-void delete_obj_refs_table(struct obj_refs_table *table) {}
-void emit_obj_refs_plain(struct obj_refs_table *table, __u32 id, const char *prefix) {}
-void emit_obj_refs_json(struct obj_refs_table *table, __u32 id, json_writer_t *json_writer) {}
+void delete_obj_refs_table(struct hashmap *map) {}
+void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix) {}
+void emit_obj_refs_json(struct hashmap *map, __u32 id, json_writer_t *json_writer) {}
 
 #else /* BPFTOOL_WITHOUT_SKELETONS */
 
 #include "pid_iter.skel.h"
 
-static void add_ref(struct obj_refs_table *table, struct pid_iter_entry *e)
+static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
 {
+	struct hashmap_entry *entry;
 	struct obj_refs *refs;
 	struct obj_ref *ref;
 	void *tmp;
 	int i;
 
-	hash_for_each_possible(table->table, refs, node, e->id) {
-		if (refs->id != e->id)
-			continue;
+	hashmap__for_each_key_entry(map, entry, u32_as_hash_field(e->id)) {
+		refs = entry->value;
 
 		for (i = 0; i < refs->ref_cnt; i++) {
 			if (refs->refs[i].pid == e->pid)
@@ -64,7 +66,6 @@ static void add_ref(struct obj_refs_table *table, struct pid_iter_entry *e)
 		return;
 	}
 
-	refs->id = e->id;
 	refs->refs = malloc(sizeof(*refs->refs));
 	if (!refs->refs) {
 		free(refs);
@@ -76,7 +77,7 @@ static void add_ref(struct obj_refs_table *table, struct pid_iter_entry *e)
 	ref->pid = e->pid;
 	memcpy(ref->comm, e->comm, sizeof(ref->comm));
 	refs->ref_cnt = 1;
-	hash_add(table->table, &refs->node, e->id);
+	hashmap__append(map, u32_as_hash_field(e->id), refs);
 }
 
 static int __printf(2, 0)
@@ -87,7 +88,7 @@ libbpf_print_none(__maybe_unused enum libbpf_print_level level,
 	return 0;
 }
 
-int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type)
+int build_obj_refs_table(struct hashmap **map, enum bpf_obj_type type)
 {
 	struct pid_iter_entry *e;
 	char buf[4096 / sizeof(*e) * sizeof(*e)];
@@ -95,7 +96,11 @@ int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type)
 	int err, ret, fd = -1, i;
 	libbpf_print_fn_t default_print;
 
-	hash_init(table->table);
+	*map = hashmap__new(bpftool_hash_fn, bpftool_equal_fn, NULL);
+	if (!*map) {
+		p_err("failed to create hashmap for PID references");
+		return -1;
+	}
 	set_max_rlimit();
 
 	skel = pid_iter_bpf__open();
@@ -151,7 +156,7 @@ int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type)
 
 		e = (void *)buf;
 		for (i = 0; i < ret; i++, e++) {
-			add_ref(table, e);
+			add_ref(*map, e);
 		}
 	}
 	err = 0;
@@ -162,39 +167,44 @@ int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type)
 	return err;
 }
 
-void delete_obj_refs_table(struct obj_refs_table *table)
+void delete_obj_refs_table(struct hashmap *map)
 {
-	struct obj_refs *refs;
-	struct hlist_node *tmp;
-	unsigned int bkt;
+	struct hashmap_entry *entry;
+	size_t bkt;
+
+	if (!map)
+		return;
+
+	hashmap__for_each_entry(map, entry, bkt) {
+		struct obj_refs *refs = entry->value;
 
-	hash_for_each_safe(table->table, bkt, tmp, refs, node) {
-		hash_del(&refs->node);
 		free(refs->refs);
 		free(refs);
 	}
+
+	hashmap__free(map);
 }
 
-void emit_obj_refs_json(struct obj_refs_table *table, __u32 id,
+void emit_obj_refs_json(struct hashmap *map, __u32 id,
 			json_writer_t *json_writer)
 {
-	struct obj_refs *refs;
-	struct obj_ref *ref;
-	int i;
+	struct hashmap_entry *entry;
 
-	if (hash_empty(table->table))
+	if (hashmap__empty(map))
 		return;
 
-	hash_for_each_possible(table->table, refs, node, id) {
-		if (refs->id != id)
-			continue;
+	hashmap__for_each_key_entry(map, entry, u32_as_hash_field(id)) {
+		struct obj_refs *refs = entry->value;
+		int i;
+
 		if (refs->ref_cnt == 0)
 			break;
 
 		jsonw_name(json_writer, "pids");
 		jsonw_start_array(json_writer);
 		for (i = 0; i < refs->ref_cnt; i++) {
-			ref = &refs->refs[i];
+			struct obj_ref *ref = &refs->refs[i];
+
 			jsonw_start_object(json_writer);
 			jsonw_int_field(json_writer, "pid", ref->pid);
 			jsonw_string_field(json_writer, "comm", ref->comm);
@@ -205,24 +215,24 @@ void emit_obj_refs_json(struct obj_refs_table *table, __u32 id,
 	}
 }
 
-void emit_obj_refs_plain(struct obj_refs_table *table, __u32 id, const char *prefix)
+void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix)
 {
-	struct obj_refs *refs;
-	struct obj_ref *ref;
-	int i;
+	struct hashmap_entry *entry;
 
-	if (hash_empty(table->table))
+	if (hashmap__empty(map))
 		return;
 
-	hash_for_each_possible(table->table, refs, node, id) {
-		if (refs->id != id)
-			continue;
+	hashmap__for_each_key_entry(map, entry, u32_as_hash_field(id)) {
+		struct obj_refs *refs = entry->value;
+		int i;
+
 		if (refs->ref_cnt == 0)
 			break;
 
 		printf("%s", prefix);
 		for (i = 0; i < refs->ref_cnt; i++) {
-			ref = &refs->refs[i];
+			struct obj_ref *ref = &refs->refs[i];
+
 			printf("%s%s(%d)", i == 0 ? "" : ", ", ref->comm, ref->pid);
 		}
 		break;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 20026b4178b0..11a250f60cd5 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -428,7 +428,7 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
 		jsonw_end_array(json_wtr);
 	}
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr);
+	emit_obj_refs_json(refs_table, info->id, json_wtr);
 
 	show_prog_metadata(fd, info->nr_map_ids);
 
@@ -499,7 +499,7 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
 	if (info->btf_id)
 		printf("\n\tbtf_id %d", info->btf_id);
 
-	emit_obj_refs_plain(&refs_table, info->id, "\n\tpids ");
+	emit_obj_refs_plain(refs_table, info->id, "\n\tpids ");
 
 	printf("\n");
 
@@ -616,7 +616,7 @@ static int do_show(int argc, char **argv)
 	if (json_output)
 		jsonw_end_array(json_wtr);
 
-	delete_obj_refs_table(&refs_table);
+	delete_obj_refs_table(refs_table);
 
 	if (show_pinned)
 		delete_pinned_obj_table(prog_table);
-- 
2.30.2


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

* Re: [PATCH bpf-next 2/5] bpftool: Do not expose and init hash maps for pinned path in main.c
  2021-10-22 17:16 ` [PATCH bpf-next 2/5] bpftool: Do not expose and init hash maps for pinned path in main.c Quentin Monnet
@ 2021-10-23  0:13   ` Andrii Nakryiko
  2021-10-23 20:50     ` Quentin Monnet
  0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2021-10-23  0:13 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> BPF programs, maps, and links, can all be listed with their pinned paths
> by bpftool, when the "-f" option is provided. To do so, bpftool builds
> hash maps containing all pinned paths for each kind of objects.
>
> These three hash maps are always initialised in main.c, and exposed
> through main.h. There appear to be no particular reason to do so: we can
> just as well make them static to the files that need them (prog.c,
> map.c, and link.c respectively), and initialise them only when we want
> to show objects and the "-f" switch is provided.
>
> This may prevent unnecessary memory allocations if the implementation of
> the hash maps was to change in the future.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/link.c |  9 ++++++++-
>  tools/bpf/bpftool/main.c | 12 ------------
>  tools/bpf/bpftool/main.h |  3 ---
>  tools/bpf/bpftool/map.c  |  9 ++++++++-
>  tools/bpf/bpftool/prog.c |  9 ++++++++-
>  5 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
> index 8cc3e36f8cc6..ebf29be747b3 100644
> --- a/tools/bpf/bpftool/link.c
> +++ b/tools/bpf/bpftool/link.c
> @@ -20,6 +20,8 @@ static const char * const link_type_name[] = {
>         [BPF_LINK_TYPE_NETNS]                   = "netns",
>  };
>
> +static struct pinned_obj_table link_table;
> +
>  static int link_parse_fd(int *argc, char ***argv)
>  {
>         int fd;
> @@ -302,8 +304,10 @@ static int do_show(int argc, char **argv)
>         __u32 id = 0;
>         int err, fd;
>
> -       if (show_pinned)
> +       if (show_pinned) {
> +               hash_init(link_table.table);
>                 build_pinned_obj_table(&link_table, BPF_OBJ_LINK);
> +       }
>         build_obj_refs_table(&refs_table, BPF_OBJ_LINK);
>
>         if (argc == 2) {
> @@ -384,6 +388,9 @@ static int do_detach(int argc, char **argv)
>         if (json_output)
>                 jsonw_null(json_wtr);
>
> +       if (show_pinned)
> +               delete_pinned_obj_table(&link_table);

Shouldn't this be in do_show rather than do_detach?

> +
>         return 0;
>  }
>

[...]

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

* Re: [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects
  2021-10-22 17:16 ` [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects Quentin Monnet
@ 2021-10-23  0:54   ` Andrii Nakryiko
  2021-10-23 20:51     ` Quentin Monnet
  0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2021-10-23  0:54 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> In order to show pinned paths for BPF programs, maps, or links when
> listing them with the "-f" option, bpftool creates hash maps to store
> all relevant paths under the bpffs. So far, it would rely on the
> kernel implementation (from tools/include/linux/hashtable.h).
>
> We can make bpftool rely on libbpf's implementation instead. The
> motivation is to make bpftool less dependent of kernel headers, to ease
> the path to a potential out-of-tree mirror, like libbpf has.
>
> This commit is the first step of the conversion: the hash maps for
> pinned paths for programs, maps, and links are converted to libbpf's
> hashmap.{c,h}. Other hash maps used for the PIDs of process holding
> references to BPF objects are left unchanged for now. On the build side,
> this requires adding a dependency to a second header internal to libbpf,
> and making it a dependency for the bootstrap bpftool version as well.
> The rest of the changes are a rather straightforward conversion.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/Makefile |  8 +++---
>  tools/bpf/bpftool/common.c | 50 ++++++++++++++++++++------------------
>  tools/bpf/bpftool/link.c   | 35 ++++++++++++++------------
>  tools/bpf/bpftool/main.h   | 29 +++++++++++++---------
>  tools/bpf/bpftool/map.c    | 35 ++++++++++++++------------
>  tools/bpf/bpftool/prog.c   | 35 ++++++++++++++------------
>  6 files changed, 105 insertions(+), 87 deletions(-)
>

[...]

> @@ -420,28 +421,20 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb,
>         if (bpf_obj_get_info_by_fd(fd, &pinned_info, &len))
>                 goto out_close;
>
> -       obj_node = calloc(1, sizeof(*obj_node));
> -       if (!obj_node) {
> +       path = strdup(fpath);
> +       if (!path) {
>                 err = -1;
>                 goto out_close;
>         }
>
> -       obj_node->id = pinned_info.id;
> -       obj_node->path = strdup(fpath);
> -       if (!obj_node->path) {
> -               err = -1;
> -               free(obj_node);
> -               goto out_close;
> -       }
> -
> -       hash_add(build_fn_table->table, &obj_node->hash, obj_node->id);
> +       hashmap__append(build_fn_table, u32_as_hash_field(pinned_info.id), path);

handle errors? operation can fail

>  out_close:
>         close(fd);
>  out_ret:
>         return err;
>  }

[...]

>
>  unsigned int get_page_size(void)
> @@ -962,3 +956,13 @@ int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len)
>
>         return fd;
>  }
> +
> +size_t bpftool_hash_fn(const void *key, void *ctx)
> +{
> +       return (size_t)key;
> +}
> +
> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx)

kind of too generic and too assuming function (hash_fn and
equal_fn)... Maybe either use static functions near each hashmap use
case, or name it to specify that it works when keys are ids?

> +{
> +       return k1 == k2;
> +}

[...]

> @@ -256,4 +247,18 @@ int do_filter_dump(struct tcmsg *ifinfo, struct nlattr **tb, const char *kind,
>
>  int print_all_levels(__maybe_unused enum libbpf_print_level level,
>                      const char *format, va_list args);
> +
> +size_t bpftool_hash_fn(const void *key, void *ctx);
> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx);
> +
> +static inline void *u32_as_hash_field(__u32 x)

it's used for keys only, right? so u32_as_hash_key?

> +{
> +       return (void *)(uintptr_t)x;
> +}
> +
> +static inline bool hashmap__empty(struct hashmap *map)
> +{
> +       return map ? hashmap__size(map) == 0 : true;
> +}
> +
>  #endif

[...]

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

* Re: [PATCH bpf-next 4/5] bpftool: Switch to libbpf's hashmap for programs/maps in BTF listing
  2021-10-22 17:16 ` [PATCH bpf-next 4/5] bpftool: Switch to libbpf's hashmap for programs/maps in BTF listing Quentin Monnet
@ 2021-10-23  1:47   ` Andrii Nakryiko
  0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2021-10-23  1:47 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> In order to show BPF programs and maps using BTF objects when the latter
> are being listed, bpftool creates hash maps to store all relevant items.
> This commit is part of a set that transitions from the kernel's hash map
> implementation to the one coming with libbpf.
>
> The motivation is to make bpftool less dependent of kernel headers, to
> ease the path to a potential out-of-tree mirror, like libbpf has.
>
> This commit focuses on the two hash maps used by bpftool when listing
> BTF objects to store references to programs and maps, and convert them
> to the libbpf's implementation.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/btf.c  | 126 ++++++++++++++++-----------------------
>  tools/bpf/bpftool/main.h |   5 ++
>  2 files changed, 57 insertions(+), 74 deletions(-)
>

[...]

> @@ -741,28 +724,20 @@ build_btf_type_table(struct btf_attach_table *tab, enum bpf_obj_type type,
>                 if (!btf_id)
>                         continue;
>
> -               obj_node = calloc(1, sizeof(*obj_node));
> -               if (!obj_node) {
> -                       p_err("failed to allocate memory: %s", strerror(errno));
> -                       err = -ENOMEM;
> -                       goto err_free;
> -               }
> -
> -               obj_node->obj_id = id;
> -               obj_node->btf_id = btf_id;
> -               hash_add(tab->table, &obj_node->hash, obj_node->btf_id);
> +               hashmap__append(tab, u32_as_hash_field(btf_id),
> +                               u32_as_hash_field(id));

error handling is missing

>         }
>
>         return 0;
>
>  err_free:
> -       delete_btf_table(tab);
> +       hashmap__free(tab);
>         return err;
>  }
>
>  static int
> -build_btf_tables(struct btf_attach_table *btf_prog_table,
> -                struct btf_attach_table *btf_map_table)
> +build_btf_tables(struct hashmap *btf_prog_table,
> +                struct hashmap *btf_map_table)
>  {
>         struct bpf_prog_info prog_info;
>         __u32 prog_len = sizeof(prog_info);
> @@ -778,7 +753,7 @@ build_btf_tables(struct btf_attach_table *btf_prog_table,
>         err = build_btf_type_table(btf_map_table, BPF_OBJ_MAP, &map_info,
>                                    &map_len);
>         if (err) {
> -               delete_btf_table(btf_prog_table);
> +               hashmap__free(btf_prog_table);
>                 return err;
>         }
>
> @@ -787,10 +762,10 @@ build_btf_tables(struct btf_attach_table *btf_prog_table,
>
>  static void
>  show_btf_plain(struct bpf_btf_info *info, int fd,
> -              struct btf_attach_table *btf_prog_table,
> -              struct btf_attach_table *btf_map_table)
> +              struct hashmap *btf_prog_table,
> +              struct hashmap *btf_map_table)
>  {
> -       struct btf_attach_point *obj;
> +       struct hashmap_entry *entry;
>         const char *name = u64_to_ptr(info->name);
>         int n;
>
> @@ -804,18 +779,17 @@ show_btf_plain(struct bpf_btf_info *info, int fd,
>         printf("size %uB", info->btf_size);
>
>         n = 0;
> -       hash_for_each_possible(btf_prog_table->table, obj, hash, info->id) {
> -               if (obj->btf_id == info->id)
> -                       printf("%s%u", n++ == 0 ? "  prog_ids " : ",",
> -                              obj->obj_id);
> -       }
> +       hashmap__for_each_key_entry(btf_prog_table, entry,
> +                                   u32_as_hash_field(info->id))
> +               printf("%s%u", n++ == 0 ? "  prog_ids " : ",",
> +                      hash_field_as_u32(entry->value));

nit: I'd add {}, it's getting a bit hard to follow

>
>         n = 0;
> -       hash_for_each_possible(btf_map_table->table, obj, hash, info->id) {
> -               if (obj->btf_id == info->id)
> -                       printf("%s%u", n++ == 0 ? "  map_ids " : ",",
> -                              obj->obj_id);
> -       }
> +       hashmap__for_each_key_entry(btf_map_table, entry,
> +                                   u32_as_hash_field(info->id))
> +               printf("%s%u", n++ == 0 ? "  map_ids " : ",",
> +                      hash_field_as_u32(entry->value));
> +
>         emit_obj_refs_plain(&refs_table, info->id, "\n\tpids ");
>
>         printf("\n");

[...]

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

* Re: [PATCH bpf-next 5/5] bpftool: Switch to libbpf's hashmap for PIDs/names references
  2021-10-22 17:16 ` [PATCH bpf-next 5/5] bpftool: Switch to libbpf's hashmap for PIDs/names references Quentin Monnet
@ 2021-10-23  1:50   ` Andrii Nakryiko
  0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2021-10-23  1:50 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> In order to show PIDs and names for processes holding references to BPF
> programs, maps, links, or BTF objects, bpftool creates hash maps to
> store all relevant information. This commit is part of a set that
> transitions from the kernel's hash map implementation to the one coming
> with libbpf.
>
> The motivation is to make bpftool less dependent of kernel headers, to
> ease the path to a potential out-of-tree mirror, like libbpf has.
>
> This is the third and final step of the transition, in which we convert
> the hash maps used for storing the information about the processes
> holding references to BPF objects (programs, maps, links, BTF), and at
> last we drop the inclusion of tools/include/linux/hashtable.h.
>
> Note: Checkpatch complains about the use of __weak declarations, and the
> missing empty lines after the bunch of empty function declarations when
> compiling without the BPF skeletons (none of these were introduced in
> this patch). We want to keep things as they are, and the reports should
> be safe to ignore.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/btf.c  |  7 ++--
>  tools/bpf/bpftool/link.c |  6 +--
>  tools/bpf/bpftool/main.c |  5 ++-
>  tools/bpf/bpftool/main.h | 17 +++-----
>  tools/bpf/bpftool/map.c  |  6 +--
>  tools/bpf/bpftool/pids.c | 84 ++++++++++++++++++++++------------------
>  tools/bpf/bpftool/prog.c |  6 +--
>  7 files changed, 67 insertions(+), 64 deletions(-)
>

[...]

>  #include "pid_iter.skel.h"
>
> -static void add_ref(struct obj_refs_table *table, struct pid_iter_entry *e)
> +static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
>  {
> +       struct hashmap_entry *entry;
>         struct obj_refs *refs;
>         struct obj_ref *ref;
>         void *tmp;
>         int i;
>
> -       hash_for_each_possible(table->table, refs, node, e->id) {
> -               if (refs->id != e->id)
> -                       continue;
> +       hashmap__for_each_key_entry(map, entry, u32_as_hash_field(e->id)) {
> +               refs = entry->value;
>
>                 for (i = 0; i < refs->ref_cnt; i++) {
>                         if (refs->refs[i].pid == e->pid)
> @@ -64,7 +66,6 @@ static void add_ref(struct obj_refs_table *table, struct pid_iter_entry *e)
>                 return;
>         }
>
> -       refs->id = e->id;
>         refs->refs = malloc(sizeof(*refs->refs));
>         if (!refs->refs) {
>                 free(refs);
> @@ -76,7 +77,7 @@ static void add_ref(struct obj_refs_table *table, struct pid_iter_entry *e)
>         ref->pid = e->pid;
>         memcpy(ref->comm, e->comm, sizeof(ref->comm));
>         refs->ref_cnt = 1;
> -       hash_add(table->table, &refs->node, e->id);
> +       hashmap__append(map, u32_as_hash_field(e->id), refs);

here as well, can fail

>  }
>
>  static int __printf(2, 0)
> @@ -87,7 +88,7 @@ libbpf_print_none(__maybe_unused enum libbpf_print_level level,
>         return 0;
>  }
>

[...]

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

* Re: [PATCH bpf-next 2/5] bpftool: Do not expose and init hash maps for pinned path in main.c
  2021-10-23  0:13   ` Andrii Nakryiko
@ 2021-10-23 20:50     ` Quentin Monnet
  0 siblings, 0 replies; 12+ messages in thread
From: Quentin Monnet @ 2021-10-23 20:50 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

2021-10-22 17:13 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> BPF programs, maps, and links, can all be listed with their pinned paths
>> by bpftool, when the "-f" option is provided. To do so, bpftool builds
>> hash maps containing all pinned paths for each kind of objects.
>>
>> These three hash maps are always initialised in main.c, and exposed
>> through main.h. There appear to be no particular reason to do so: we can
>> just as well make them static to the files that need them (prog.c,
>> map.c, and link.c respectively), and initialise them only when we want
>> to show objects and the "-f" switch is provided.
>>
>> This may prevent unnecessary memory allocations if the implementation of
>> the hash maps was to change in the future.
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> ---
>>  tools/bpf/bpftool/link.c |  9 ++++++++-
>>  tools/bpf/bpftool/main.c | 12 ------------
>>  tools/bpf/bpftool/main.h |  3 ---
>>  tools/bpf/bpftool/map.c  |  9 ++++++++-
>>  tools/bpf/bpftool/prog.c |  9 ++++++++-
>>  5 files changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
>> index 8cc3e36f8cc6..ebf29be747b3 100644
>> --- a/tools/bpf/bpftool/link.c
>> +++ b/tools/bpf/bpftool/link.c
>> @@ -20,6 +20,8 @@ static const char * const link_type_name[] = {
>>         [BPF_LINK_TYPE_NETNS]                   = "netns",
>>  };
>>
>> +static struct pinned_obj_table link_table;
>> +
>>  static int link_parse_fd(int *argc, char ***argv)
>>  {
>>         int fd;
>> @@ -302,8 +304,10 @@ static int do_show(int argc, char **argv)
>>         __u32 id = 0;
>>         int err, fd;
>>
>> -       if (show_pinned)
>> +       if (show_pinned) {
>> +               hash_init(link_table.table);
>>                 build_pinned_obj_table(&link_table, BPF_OBJ_LINK);
>> +       }
>>         build_obj_refs_table(&refs_table, BPF_OBJ_LINK);
>>
>>         if (argc == 2) {
>> @@ -384,6 +388,9 @@ static int do_detach(int argc, char **argv)
>>         if (json_output)
>>                 jsonw_null(json_wtr);
>>
>> +       if (show_pinned)
>> +               delete_pinned_obj_table(&link_table);
> 
> Shouldn't this be in do_show rather than do_detach?

Yikes! How did it end up here? Thanks for the catch, sure, I'll fix it.

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

* Re: [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects
  2021-10-23  0:54   ` Andrii Nakryiko
@ 2021-10-23 20:51     ` Quentin Monnet
  0 siblings, 0 replies; 12+ messages in thread
From: Quentin Monnet @ 2021-10-23 20:51 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

2021-10-22 17:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> In order to show pinned paths for BPF programs, maps, or links when
>> listing them with the "-f" option, bpftool creates hash maps to store
>> all relevant paths under the bpffs. So far, it would rely on the
>> kernel implementation (from tools/include/linux/hashtable.h).
>>
>> We can make bpftool rely on libbpf's implementation instead. The
>> motivation is to make bpftool less dependent of kernel headers, to ease
>> the path to a potential out-of-tree mirror, like libbpf has.
>>
>> This commit is the first step of the conversion: the hash maps for
>> pinned paths for programs, maps, and links are converted to libbpf's
>> hashmap.{c,h}. Other hash maps used for the PIDs of process holding
>> references to BPF objects are left unchanged for now. On the build side,
>> this requires adding a dependency to a second header internal to libbpf,
>> and making it a dependency for the bootstrap bpftool version as well.
>> The rest of the changes are a rather straightforward conversion.
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> ---
>>  tools/bpf/bpftool/Makefile |  8 +++---
>>  tools/bpf/bpftool/common.c | 50 ++++++++++++++++++++------------------
>>  tools/bpf/bpftool/link.c   | 35 ++++++++++++++------------
>>  tools/bpf/bpftool/main.h   | 29 +++++++++++++---------
>>  tools/bpf/bpftool/map.c    | 35 ++++++++++++++------------
>>  tools/bpf/bpftool/prog.c   | 35 ++++++++++++++------------
>>  6 files changed, 105 insertions(+), 87 deletions(-)
>>
> 
> [...]
> 
>> @@ -420,28 +421,20 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb,
>>         if (bpf_obj_get_info_by_fd(fd, &pinned_info, &len))
>>                 goto out_close;
>>
>> -       obj_node = calloc(1, sizeof(*obj_node));
>> -       if (!obj_node) {
>> +       path = strdup(fpath);
>> +       if (!path) {
>>                 err = -1;
>>                 goto out_close;
>>         }
>>
>> -       obj_node->id = pinned_info.id;
>> -       obj_node->path = strdup(fpath);
>> -       if (!obj_node->path) {
>> -               err = -1;
>> -               free(obj_node);
>> -               goto out_close;
>> -       }
>> -
>> -       hash_add(build_fn_table->table, &obj_node->hash, obj_node->id);
>> +       hashmap__append(build_fn_table, u32_as_hash_field(pinned_info.id), path);
> 
> handle errors? operation can fail

Right, I missed it for hashmap__append(). I'll address everywhere.

> 
>>  out_close:
>>         close(fd);
>>  out_ret:
>>         return err;
>>  }
> 
> [...]
> 
>>
>>  unsigned int get_page_size(void)
>> @@ -962,3 +956,13 @@ int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len)
>>
>>         return fd;
>>  }
>> +
>> +size_t bpftool_hash_fn(const void *key, void *ctx)
>> +{
>> +       return (size_t)key;
>> +}
>> +
>> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx)
> 
> kind of too generic and too assuming function (hash_fn and
> equal_fn)... Maybe either use static functions near each hashmap use
> case, or name it to specify that it works when keys are ids?

Makes sense. I'll probably just rename them in that case, because the
functions are the same for all hash maps and I don't really feel like
having five copies of it.

> 
>> +{
>> +       return k1 == k2;
>> +}
> 
> [...]
> 
>> @@ -256,4 +247,18 @@ int do_filter_dump(struct tcmsg *ifinfo, struct nlattr **tb, const char *kind,
>>
>>  int print_all_levels(__maybe_unused enum libbpf_print_level level,
>>                      const char *format, va_list args);
>> +
>> +size_t bpftool_hash_fn(const void *key, void *ctx);
>> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx);
>> +
>> +static inline void *u32_as_hash_field(__u32 x)
> 
> it's used for keys only, right? so u32_as_hash_key?

Yes for this patch, but we're calling it on a value in the following
patch, in build_btf_type_table(), to store the ID of a program or a map
associated to a given BTF object ID.

So I'll keep the current name here for v2, but I'll address all your
other comments.

Thanks for the review,
Quentin


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

end of thread, other threads:[~2021-10-23 20:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 17:16 [PATCH bpf-next 0/5] bpftool: Switch to libbpf's hashmap for referencing BPF objects Quentin Monnet
2021-10-22 17:16 ` [PATCH bpf-next 1/5] bpftool: Remove Makefile dep. on $(LIBBPF) for $(LIBBPF_INTERNAL_HDRS) Quentin Monnet
2021-10-22 17:16 ` [PATCH bpf-next 2/5] bpftool: Do not expose and init hash maps for pinned path in main.c Quentin Monnet
2021-10-23  0:13   ` Andrii Nakryiko
2021-10-23 20:50     ` Quentin Monnet
2021-10-22 17:16 ` [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects Quentin Monnet
2021-10-23  0:54   ` Andrii Nakryiko
2021-10-23 20:51     ` Quentin Monnet
2021-10-22 17:16 ` [PATCH bpf-next 4/5] bpftool: Switch to libbpf's hashmap for programs/maps in BTF listing Quentin Monnet
2021-10-23  1:47   ` Andrii Nakryiko
2021-10-22 17:16 ` [PATCH bpf-next 5/5] bpftool: Switch to libbpf's hashmap for PIDs/names references Quentin Monnet
2021-10-23  1:50   ` Andrii Nakryiko

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