Re: [PATCH bpf-next v3 1/3] libbpf: hashmap interface update to long -> long

[Eduard Zingerman <eddyz87@gmail.com>]

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@gmail.com> wrote:
> > 
> > On Sun, Nov 6, 2022 at 12:29 PM Eduard Zingerman <eddyz87@gmail.com> 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@meta.com/T/#m65b28f1d6d969fcd318b556db6a3ad499a42607d
> > > 
> > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > ---
> > >  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)
> $