Re: [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects

2021-10-22 17:54 UTC-0700 ~ Andrii Nakryiko [off-list ref]

On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet [off-list ref] wrote:

quoted

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

[...]

quoted

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

quoted

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

[...]

quoted

 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.

quoted

+{
+       return k1 == k2;
+}

[...]

quoted

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