Message-ID: <1D69F857-BCD9-40C1-87CA-90C55B42984D@fb.com>
Date:   Tue, 7 Dec 2021 23:10:36 +0000
From:   Song Liu <songliubraving@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.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 Dec 7, 2021, at 3:02 PM, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> 
> 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)?

Arnaldo is working on this. I guess we can flip the default soon. 

> 
>> 
>>> 
>>>> 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).

OK. I will send this version. And we can decide the next step when we
remove bpf_create_map(). 

Thanks,
Song

Powered by blists - more mailing lists