On Mon, 2022-11-07 at 16:46 -0800, Andrii Nakryiko wrote: > On Sun, Nov 6, 2022 at 12:43 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Sun, Nov 6, 2022 at 12:29 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > > > > An update for libbpf's hashmap interface from void* -> void* to > > > long -> long. Removes / simplifies some type casts when hashmap > > > keys or values are 32-bit integers. > > > > > > In libbpf hashmap is more often used with integral keys / values > > > rather than with pointer keys / values. > > > > > > 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. > > > > > > The net number of casts is decreased after this refactoring. Although > > > perf mostly uses ptr to ptr maps, thus a lot of casts have to be > > > added there: > > > > > > Casts Casts > > > removed added > > > libbpf ~50 ~20 > > > libbpf tests ~55 ~0 > > > perf ~0 ~33 > > > perf tests ~0 ~13 > > > > > > This is a follow up for [1]. > > > > > > [1] 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 | 16 +-- > > > tools/lib/bpf/hashmap.c | 16 +-- > > > tools/lib/bpf/hashmap.h | 34 +++--- > > > tools/lib/bpf/libbpf.c | 18 ++-- > > > tools/lib/bpf/strset.c | 18 ++-- > > > tools/lib/bpf/usdt.c | 31 +++--- > > > tools/perf/tests/expr.c | 40 +++---- > > > tools/perf/tests/pmu-events.c | 6 +- > > > tools/perf/util/bpf-loader.c | 23 ++-- > > > tools/perf/util/expr.c | 32 +++--- > > > tools/perf/util/hashmap.c | 16 +-- > > > tools/perf/util/hashmap.h | 34 +++--- > > > tools/perf/util/metricgroup.c | 12 +-- > > > tools/perf/util/stat.c | 9 +- > > > .../selftests/bpf/prog_tests/hashmap.c | 102 +++++++++--------- > > > .../bpf/prog_tests/kprobe_multi_test.c | 6 +- > > > 25 files changed, 257 insertions(+), 305 deletions(-) > > > > Looks like the churn is not worth it. > > I'd keep it as-is. > > No-no, this is already a big win for libbpf/bpftool as is, but we can > do even better for perf and some uses in selftest and libbpf itself. > Given a hashmap can be used with a pointer or an integer as the > key/value, we can use a bit of smartness (while keeping the safety) > through simple macro wrapper for operations like hashmap__find and > hashmap__insert (and co). That will avoid most of if not all casts for > hashmap lookups/updates. And then for hashmap__for_each_entry and > such, we can define hashmap_entry to have a union of long-based and > void*-based key: > > struct hashmap_entry { > union { > long key; > const void *pkey; > }; > union { > long value; > void *pvalue; > }; > ... > } > > I have it a try in few perf places, and it allows to avoid all the > casts while thanks to that _Statis_assert we should be even safer than > it was before. Eduard, please check the diff below and see if you can > incorporate similar changes for other operations, if necessary. > That's a nice hack, thank you. Everything works after interface function / macro update. I need a bit more time to wrap up the patch-set, will post it tomorrow. > $ git diff > diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c > index dfe99e766f30..0c1c1289a694 100644 > --- a/tools/lib/bpf/hashmap.c > +++ b/tools/lib/bpf/hashmap.c > @@ -203,7 +203,7 @@ int hashmap__insert(struct hashmap *map, long key, > long value, > return 0; > } > > -bool hashmap__find(const struct hashmap *map, long key, long *value) > +bool hashmap_find(const struct hashmap *map, long key, long *value) > { > struct hashmap_entry *entry; > size_t h; > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > index 7393ef616920..daec29012808 100644 > --- a/tools/lib/bpf/hashmap.h > +++ b/tools/lib/bpf/hashmap.h > @@ -47,8 +47,14 @@ typedef size_t (*hashmap_hash_fn)(long key, void *ctx); > typedef bool (*hashmap_equal_fn)(long key1, long key2, void *ctx); > > struct hashmap_entry { > - long key; > - long value; > + union { > + long key; > + const void *pkey; > + }; > + union { > + long value; > + void *pvalue; > + }; > struct hashmap_entry *next; > }; > > @@ -144,7 +150,13 @@ static inline int hashmap__append(struct hashmap > *map, long key, long value) > > bool hashmap__delete(struct hashmap *map, long key, long *old_key, > long *old_value); > > -bool hashmap__find(const struct hashmap *map, long key, long *value); > +bool hashmap_find(const struct hashmap *map, long key, long *value); > + > +#define hashmap__find(map, key, value) ({ > \ > + _Static_assert(value == NULL || sizeof(*value) == > sizeof(long), \ > + "Value pointee should be a long-sized > integer or a pointer"); \ > + hashmap_find(map, (long)key, (long *)value); > \ > +}) > > /* > * hashmap__for_each_entry - iterate over all entries in hashmap > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 96092c9cb34b..1a1a76357f72 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -5669,7 +5669,7 @@ static int bpf_core_resolve_relo(struct bpf_program *prog, > return -EINVAL; > > if (relo->kind != BPF_CORE_TYPE_ID_LOCAL && > - !hashmap__find(cand_cache, local_id, (long *)&cands)) { > + !hashmap__find(cand_cache, local_id, &cands)) { > cands = bpf_core_find_cands(prog->obj, local_btf, local_id); > if (IS_ERR(cands)) { > pr_warn("prog '%s': relo #%d: target candidate > search failed for [%d] %s %s: %ld\n", > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c > index 8107ed0428c2..cb206003c1f0 100644 > --- a/tools/perf/tests/expr.c > +++ b/tools/perf/tests/expr.c > @@ -130,12 +130,9 @@ static int test__expr(struct test_suite *t > __maybe_unused, int subtest __maybe_u > expr__find_ids("FOO + BAR + BAZ + BOZO", "FOO", > ctx) == 0); > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 3); > - TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, (long)"BAR", > - (long *)&val_ptr)); > - TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, (long)"BAZ", > - (long *)&val_ptr)); > - TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, (long)"BOZO", > - (long *)&val_ptr)); > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"BAR", &val_ptr)); > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"BAZ", &val_ptr)); > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"BOZO", &val_ptr)); > > expr__ctx_clear(ctx); > ctx->sctx.runtime = 3; > @@ -143,20 +140,16 @@ static int test__expr(struct test_suite *t > __maybe_unused, int subtest __maybe_u > expr__find_ids("EVENT1\\,param\\=?@ + > EVENT2\\,param\\=?@", > NULL, ctx) == 0); > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 2); > - TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"EVENT1,param=3@", > - (long *)&val_ptr)); > - TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"EVENT2,param=3@", > - (long *)&val_ptr)); > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"EVENT1,param=3@", &val_ptr)); > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"EVENT2,param=3@", &val_ptr)); > > expr__ctx_clear(ctx); > TEST_ASSERT_VAL("find ids", > expr__find_ids("dash\\-event1 - dash\\-event2", > NULL, ctx) == 0); > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 2); > - TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, (long)"dash-event1", > - (long *)&val_ptr)); > - TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, (long)"dash-event2", > - (long *)&val_ptr)); > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"dash-event1", &val_ptr)); > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)"dash-event2", &val_ptr)); > > /* Only EVENT1 or EVENT2 need be measured depending on the > value of smt_on. */ > { > @@ -174,7 +167,7 @@ static int test__expr(struct test_suite *t > __maybe_unused, int subtest __maybe_u > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1); > TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > (long)(smton > ? "EVENT1" : "EVENT2"), > - (long *)&val_ptr)); > + &val_ptr)); > > expr__ctx_clear(ctx); > TEST_ASSERT_VAL("find ids", > @@ -183,7 +176,7 @@ static int test__expr(struct test_suite *t > __maybe_unused, int subtest __maybe_u > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1); > TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, > > (long)(corewide ? "EVENT1" : "EVENT2"), > - (long *)&val_ptr)); > + &val_ptr)); > > } > /* The expression is a constant 1.0 without needing to > evaluate EVENT1. */ > @@ -220,8 +213,7 @@ static int test__expr(struct test_suite *t > __maybe_unused, int subtest __maybe_u > expr__find_ids("source_count(EVENT1)", > NULL, ctx) == 0); > TEST_ASSERT_VAL("source count", hashmap__size(ctx->ids) == 1); > - TEST_ASSERT_VAL("source count", hashmap__find(ctx->ids, (long)"EVENT1", > - (long *)&val_ptr)); > + TEST_ASSERT_VAL("source count", hashmap__find(ctx->ids, > (long)"EVENT1", &val_ptr)); > > expr__ctx_free(ctx); > > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c > index 55f114450316..2430b8965268 100644 > --- a/tools/perf/util/bpf-loader.c > +++ b/tools/perf/util/bpf-loader.c > @@ -131,7 +131,7 @@ static void *program_priv(const struct bpf_program *prog) > > if (IS_ERR_OR_NULL(bpf_program_hash)) > return NULL; > - if (!hashmap__find(bpf_program_hash, (long)prog, (long *)&priv)) > + if (!hashmap__find(bpf_program_hash, prog, &priv)) > return NULL; > return priv; > } > @@ -1170,7 +1170,7 @@ static void *map_priv(const struct bpf_map *map) > > if (IS_ERR_OR_NULL(bpf_map_hash)) > return NULL; > - if (!hashmap__find(bpf_map_hash, (long)map, (long *)&priv)) > + if (!hashmap__find(bpf_map_hash, map, &priv)) > return NULL; > return priv; > } > @@ -1184,7 +1184,7 @@ static void bpf_map_hash_free(void) > return; > > hashmap__for_each_entry(bpf_map_hash, cur, bkt) > - bpf_map_priv__clear((struct bpf_map *)cur->key, (void > *)cur->value); > + bpf_map_priv__clear(cur->pkey, cur->pvalue); > > hashmap__free(bpf_map_hash); > bpf_map_hash = NULL; > diff --git a/tools/perf/util/hashmap.c b/tools/perf/util/hashmap.c > index dfe99e766f30..0c1c1289a694 100644 > --- a/tools/perf/util/hashmap.c > +++ b/tools/perf/util/hashmap.c > @@ -203,7 +203,7 @@ int hashmap__insert(struct hashmap *map, long key, > long value, > return 0; > } > > -bool hashmap__find(const struct hashmap *map, long key, long *value) > +bool hashmap_find(const struct hashmap *map, long key, long *value) > { > struct hashmap_entry *entry; > size_t h; > diff --git a/tools/perf/util/hashmap.h b/tools/perf/util/hashmap.h > index 7393ef616920..edbadb712725 100644 > --- a/tools/perf/util/hashmap.h > +++ b/tools/perf/util/hashmap.h > @@ -47,8 +47,14 @@ typedef size_t (*hashmap_hash_fn)(long key, void *ctx); > typedef bool (*hashmap_equal_fn)(long key1, long key2, void *ctx); > > struct hashmap_entry { > - long key; > - long value; > + union { > + long key; > + const void *pkey; > + }; > + union { > + long value; > + void *pvalue; > + }; > struct hashmap_entry *next; > }; > > @@ -144,7 +150,13 @@ static inline int hashmap__append(struct hashmap > *map, long key, long value) > > bool hashmap__delete(struct hashmap *map, long key, long *old_key, > long *old_value); > > -bool hashmap__find(const struct hashmap *map, long key, long *value); > +bool hashmap_find(const struct hashmap *map, long key, long *value); > + > +#define hashmap__find(map, key, value) ({ > \ > + _Static_assert(sizeof(*value) == sizeof(long), > \ > + "Value pointee should be a long-sized > integer or a pointer"); \ > + hashmap_find(map, (long)key, (long *)value); > \ > +}) > > /* > * hashmap__for_each_entry - iterate over all entries in hashmap > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > index e9bd881c6912..2059ed3164ae 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -288,7 +288,7 @@ static int setup_metric_events(struct hashmap *ids, > * combined or shared groups, this metric may not care > * about this event. > */ > - if (hashmap__find(ids, (long)metric_id, (long *)&val_ptr)) { > + if (hashmap__find(ids, metric_id, &val_ptr)) { > metric_events[matched_events++] = ev; > > if (matched_events >= ids_size) > 11/07 16:45:42.699 andriin@devbig019:~/linux/tools/lib/bpf (master) > $
