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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2021-10-22 17:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>>  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




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux