On Wed, Nov 9, 2022 at 6:26 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> An update for libbpf's hashmap interface from void* -> void* to a
> polymorphic one, allowing both long and void* keys and values.
>
> This simplifies many use cases in libbpf as hashmaps there are mostly
> integer to integer.
>
> Perf copies hashmap implementation from libbpf and has to be
> updated as well.
>
> Changes to libbpf, selftests/bpf and perf are packed as a single
> commit to avoid compilation issues with any future bisect.
>
> Polymorphic interface is acheived by hiding hashmap interface
> functions behind auxiliary macros that take care of necessary
> type casts, for example:
>
> #define hashmap_cast_ptr(p) \
> ({ \
> _Static_assert((p) == NULL || sizeof(*(p)) == sizeof(long),\
> #p " pointee should be a long-sized integer or a pointer"); \
> (long *)(p); \
> })
>
> bool hashmap_find(const struct hashmap *map, long key, long *value);
>
> #define hashmap__find(map, key, value) \
> hashmap_find((map), (long)(key), hashmap_cast_ptr(value))
>
> - hashmap__find macro casts key and value parameters to long
> and long* respectively
> - hashmap_cast_ptr ensures that value pointer points to a memory
> of appropriate size.
>
> This hack was suggested by Andrii Nakryiko in [1].
> This is a follow up for [2].
>
> [1] https://lore.kernel.org/bpf/CAEf4BzZ8KFneEJxFAaNCCFPGqp20hSpS2aCj76uRk3-qZUH5xg@xxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/bpf/af1facf9-7bc8-8a3d-0db4-7b3f333589a2@xxxxxxxx/T/#m65b28f1d6d969fcd318b556db6a3ad499a42607d
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> tools/bpf/bpftool/btf.c | 25 +--
> tools/bpf/bpftool/common.c | 10 +-
> tools/bpf/bpftool/gen.c | 19 +-
> tools/bpf/bpftool/link.c | 8 +-
> tools/bpf/bpftool/main.h | 14 +-
> tools/bpf/bpftool/map.c | 8 +-
> tools/bpf/bpftool/pids.c | 16 +-
> tools/bpf/bpftool/prog.c | 8 +-
> tools/lib/bpf/btf.c | 41 ++--
> tools/lib/bpf/btf_dump.c | 17 +-
> tools/lib/bpf/hashmap.c | 18 +-
> tools/lib/bpf/hashmap.h | 91 +++++----
> tools/lib/bpf/libbpf.c | 18 +-
> tools/lib/bpf/strset.c | 18 +-
> tools/lib/bpf/usdt.c | 29 ++-
> tools/perf/tests/expr.c | 28 +--
> tools/perf/tests/pmu-events.c | 6 +-
> tools/perf/util/bpf-loader.c | 11 +-
> tools/perf/util/evsel.c | 2 +-
> tools/perf/util/expr.c | 36 ++--
> tools/perf/util/hashmap.c | 18 +-
> tools/perf/util/hashmap.h | 91 +++++----
> tools/perf/util/metricgroup.c | 10 +-
> tools/perf/util/stat-shadow.c | 2 +-
> tools/perf/util/stat.c | 9 +-
> .../selftests/bpf/prog_tests/hashmap.c | 190 +++++++++++++-----
would be better if you added new tests in separate patch and didn't
use CHECK(), but oh well, we'll improve that some time in the future
But regardless this is a pretty clear win, thanks a lot for working on
this! I made a few pedantic changes mentioned below, and applied to
bpf-next.
> .../bpf/prog_tests/kprobe_multi_test.c | 6 +-
> 27 files changed, 411 insertions(+), 338 deletions(-)
>
[...]
> @@ -545,7 +545,7 @@ void delete_pinned_obj_table(struct hashmap *map)
> return;
>
> hashmap__for_each_entry(map, entry, bkt)
> - free(entry->value);
> + free((void *)entry->value);
entry->pvalue
>
> hashmap__free(map);
> }
[...]
> @@ -309,8 +308,7 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
> if (!hashmap__empty(link_table)) {
> struct hashmap_entry *entry;
>
> - hashmap__for_each_key_entry(link_table, entry,
> - u32_as_hash_field(info->id))
> + hashmap__for_each_key_entry(link_table, entry, info->id)
> printf("\n\tpinned %s", (char *)entry->value);
(char *)entry->pvalue for consistent use of pvalue
> }
> emit_obj_refs_plain(refs_table, info->id, "\n\tpids ");
[...]
> @@ -595,8 +594,7 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
> if (!hashmap__empty(map_table)) {
> struct hashmap_entry *entry;
>
> - hashmap__for_each_key_entry(map_table, entry,
> - u32_as_hash_field(info->id))
> + hashmap__for_each_key_entry(map_table, entry, info->id)
> printf("\n\tpinned %s", (char *)entry->value);
same, pvalue for consistency
> }
>
[...]
> @@ -561,8 +560,7 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
> if (!hashmap__empty(prog_table)) {
> struct hashmap_entry *entry;
>
> - hashmap__for_each_key_entry(prog_table, entry,
> - u32_as_hash_field(info->id))
> + hashmap__for_each_key_entry(prog_table, entry, info->id)
> printf("\n\tpinned %s", (char *)entry->value);
ditto
> }
>
[...]
> @@ -1536,18 +1536,17 @@ static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
> const char *orig_name)
> {
> char *old_name, *new_name;
> - size_t dup_cnt = 0;
> + long dup_cnt = 0;
size_t is fine as is, right?
> int err;
>
> new_name = strdup(orig_name);
> if (!new_name)
> return 1;
>
[...]
> @@ -102,6 +122,13 @@ enum hashmap_insert_strategy {
> HASHMAP_APPEND,
> };
>
> +#define hashmap_cast_ptr(p) \
> + ({ \
> + _Static_assert((p) == NULL || sizeof(*(p)) == sizeof(long),\
> + #p " pointee should be a long-sized integer or a pointer"); \
> + (long *)(p); \
> + })
> +
I've reformatted this slightly, making it less indented to the right
> /*
> * hashmap__insert() adds key/value entry w/ various semantics, depending on
> * provided strategy value. If a given key/value pair replaced already
[...]
