Message-ID: <CAEf4BzaCioWRGktgk1TvdyaB-zF_6Hyj+67j7DzPzTLGqkigYg@mail.gmail.com>
Date:   Tue, 7 Dec 2021 15:02:22 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Song Liu <songliubraving@...com>
Cc:     Song Liu <song@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Kernel Team <Kernel-team@...com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH bpf-next] perf/bpf_counter: use bpf_map_create instead of bpf_create_map

On Mon, Dec 6, 2021 at 10:30 PM Song Liu <songliubraving@...com> wrote:
>
>
>
> > On Dec 6, 2021, at 9:13 PM, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >
> > On Mon, Dec 6, 2021 at 8:32 PM Song Liu <songliubraving@...com> wrote:
> >>
> >>
> >>
> >>> On Dec 6, 2021, at 6:37 PM, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >>>
> >>> On Mon, Dec 6, 2021 at 3:08 PM Song Liu <song@...nel.org> wrote:
> >>>>
> >>>> 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?

I see, right, I think I already asked about that before :( Is it
possible to set Makefile such that it will do BUILD_BPF_SKEL=1 if
Clang version is recent enough and other conditions are satisfied
(unless overridden or something)?

>
> >
> >> 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.
> >
> >>
> >>>
> >>>> Signed-off-by: Song Liu <song@...nel.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?

Yes and yes. I'm not sure what would be perf's plan w.r.t. libbpf 1.0,
we'll need to work together to figure this out. At some point perf
will need to say that the minimum version of supported libbpf is v0.6
or something and just assume all those newer APIs are there (no need
to bump it all the way to libbpf 1.0, btw).

>
> Thanks,
> Song

Powered by blists - more mailing lists