Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220224155238.714682-1-jolsa@kernel.org> <20220224155238.714682-3-jolsa@kernel.org>
In-Reply-To: <20220224155238.714682-3-jolsa@kernel.org>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Mon, 28 Feb 2022 17:49:32 -0800
Message-ID: <CAEf4BzbqbHDzGJJu8Zp1QPa-bxT_usespzs=rEj2UX0A-k0gPA@mail.gmail.com>
Subject: Re: [PATCH 2/2] perf tools: Remove bpf_map__set_priv/bpf_map__priv usage
To:     Jiri Olsa <jolsa@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Andrii Nakryiko <andrii@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Ingo Molnar <mingo@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Ian Rogers <irogers@google.com>,
        "linux-perf-use." <linux-perf-users@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 24, 2022 at 7:53 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Both bpf_map__set_priv/bpf_map__priv are deprecated
> and will be eventually removed.
>
> Using hashmap to replace that functionality.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/bpf-loader.c | 66 ++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index b9d4278895ec..4f6173756a9d 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -27,6 +27,7 @@
>  #include "llvm-utils.h"
>  #include "c++/clang-c.h"
>  #include "hashmap.h"
> +#include "asm/bug.h"
>
>  #include <internal/xyarray.h>
>
> @@ -57,6 +58,7 @@ struct bpf_perf_object {
>
>  static LIST_HEAD(bpf_objects_list);
>  static struct hashmap *bpf_program_hash;
> +static struct hashmap *bpf_map_hash;
>
>  static struct bpf_perf_object *
>  bpf_perf_object__next(struct bpf_perf_object *prev)
> @@ -204,6 +206,8 @@ static void bpf_program_hash_free(void)
>         bpf_program_hash = NULL;
>  }
>
> +static void bpf_map_hash_free(void);
> +
>  void bpf__clear(void)
>  {
>         struct bpf_perf_object *perf_obj, *tmp;
> @@ -214,6 +218,7 @@ void bpf__clear(void)
>         }
>
>         bpf_program_hash_free();
> +       bpf_map_hash_free();
>  }
>
>  static size_t ptr_hash(const void *__key, void *ctx __maybe_unused)
> @@ -976,7 +981,7 @@ bpf_map_priv__purge(struct bpf_map_priv *priv)
>  }
>
>  static void
> -bpf_map_priv__clear(struct bpf_map *map __maybe_unused,
> +bpf_map_priv__clear(const struct bpf_map *map __maybe_unused,
>                     void *_priv)
>  {
>         struct bpf_map_priv *priv = _priv;
> @@ -985,6 +990,53 @@ bpf_map_priv__clear(struct bpf_map *map __maybe_unused,
>         free(priv);
>  }
>
> +static void *map_priv(const struct bpf_map *map)
> +{
> +       void *priv;
> +
> +       if (IS_ERR_OR_NULL(bpf_map_hash))
> +               return NULL;
> +       if (!hashmap__find(bpf_map_hash, map, &priv))
> +               return NULL;
> +       return priv;
> +}
> +
> +static void bpf_map_hash_free(void)
> +{
> +       struct hashmap_entry *cur;
> +       size_t bkt;
> +
> +       if (IS_ERR_OR_NULL(bpf_map_hash))
> +               return;
> +
> +       hashmap__for_each_entry(bpf_map_hash, cur, bkt)
> +               bpf_map_priv__clear(cur->key, cur->value);
> +
> +       hashmap__free(bpf_map_hash);
> +       bpf_map_hash = NULL;
> +}
> +
> +static int map_set_priv(struct bpf_map *map, void *priv)
> +{
> +       void *old_priv;
> +
> +       if (WARN_ON_ONCE(IS_ERR(bpf_map_hash)))
> +               return PTR_ERR(bpf_program_hash);
> +

you didn't warn for this in the previous patch. I have no preference,
just pointing out assymetry.

> +       if (!bpf_map_hash) {
> +               bpf_map_hash = hashmap__new(ptr_hash, ptr_equal, NULL);
> +               if (IS_ERR(bpf_map_hash))
> +                       return PTR_ERR(bpf_map_hash);
> +       }
> +
> +       old_priv = map_priv(map);
> +       if (old_priv) {
> +               bpf_map_priv__clear(map, old_priv);
> +               return hashmap__set(bpf_map_hash, map, priv, NULL, NULL);
> +       }
> +       return hashmap__add(bpf_map_hash, map, priv);
> +}
> +

[...]