Re: [PATCH bpf-next] perf/bpf_counter: use bpf_map_create instead of bpf_create_map

On Dec 6, 2021, at 9:13 PM, Andrii Nakryiko [off-list ref] wrote:

On Mon, Dec 6, 2021 at 8:32 PM Song Liu [off-list ref] wrote:

quoted

quoted

On Dec 6, 2021, at 6:37 PM, Andrii Nakryiko [off-list ref] wrote:

On Mon, Dec 6, 2021 at 3:08 PM Song Liu [off-list ref] wrote:

quoted

bpf_create_map is deprecated. Replace it with bpf_map_create.

Fixes: 992c4225419a ("libbpf: Unify low-level map creation APIs w/ new bpf_map_create()")

This is not a bug fix, it's an improvement. So I don't think "Fixes: "
is warranted here, tbh.

I got compilation errors before this change, like

util/bpf_counter.c: In function ‘bperf_lock_attr_map’:
util/bpf_counter.c:323:3: error: ‘bpf_create_map’ is deprecated: libbpf v0.7+: use bpf_map_create() instead [-Werror=deprecated-declarations]
  map_fd = bpf_create_map(BPF_MAP_TYPE_HASH,
  ^~~~~~
In file included from util/bpf_counter.h:7,
                from util/bpf_counter.c:15:
/data/users/songliubraving/kernel/linux-git/tools/lib/bpf/bpf.h:91:16: note: declared here
LIBBPF_API int bpf_create_map(enum bpf_map_type map_type, int key_size,
               ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/data/users/songliubraving/kernel/linux-git/tools/build/Makefile.build:96: util/bpf_counter.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [/data/users/songliubraving/kernel/linux-git/tools/build/Makefile.build:139: util] Error 2
make[2]: *** [Makefile.perf:665: perf-in.o] Error 2
make[1]: *** [Makefile.perf:240: sub-make] Error 2
make: *** [Makefile:70: all] Error 2

Hmm.. is util/bpf_counter.h guarded behind some Makefile arguments?
I've sent #pragma temporary workarounds just a few days ago ([0]), but
this one didn't come up during the build.

 [0] https://patchwork.kernel.org/project/netdevbpf/patch/20211203004640.2455717-1-andrii@kernel.org/

I guess the default build test doesn't enable BUILD_BPF_SKEL? 

quoted

Do we plan to remove bpf_create_map in the future? If not, we can probably just
add '#pragma GCC diagnostic ignored "-Wdeprecated-declarations"' can call it done?

Yes, it will be removed in a few libbpf releases when we switch to the
1.0 version. So suppressing a warning is a temporary work-around.

quoted

quoted

quoted

Signed-off-by: Song Liu <song@kernel.org>
---
tools/perf/util/bpf_counter.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index c17d4a43ce065..ed150a9b3a0c0 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c

@@ -320,10 +320,10 @@ static int bperf_lock_attr_map(struct target *target)
      }

      if (access(path, F_OK)) {
-               map_fd = bpf_create_map(BPF_MAP_TYPE_HASH,
+               map_fd = bpf_map_create(BPF_MAP_TYPE_HASH, NULL,

I think perf is trying to be linkable with libbpf as a shared library,
so on some older versions of libbpf bpf_map_create() won't be (yet)
available. So to make this work, I think you'll need to define your
own weak bpf_map_create function that will use bpf_create_map().

Hmm... I didn't know the plan to link libbpf as shared library. In this case,
maybe the #pragma solution is preferred?

See "perf tools: Add more weak libbpf functions" sent by Jiri not so
long ago about what they did with some other used APIs that are now
marked deprecated.

Do you mean something like this?

int __weak
bpf_map_create(enum bpf_map_type map_type,
               const char *map_name __maybe_unused,
               __u32 key_size,
               __u32 value_size,
               __u32 max_entries,
               const struct bpf_map_create_opts *opts __maybe_unused)
{
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
        return bpf_create_map(map_type, key_size, value_size, max_entries, 0);
#pragma GCC diagnostic pop
}

I guess this won't work when bpf_create_map() is eventually removed, as 
__weak function are still compiled, no?

Thanks,
Song