Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Quentin Monnet <quentin@isovalent.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	John Fastabend <john.fastabend@gmail.com>
Subject: Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()
Date: Fri, 23 Jul 2021 10:52:41 +0100	[thread overview]
Message-ID: <3802e42d-321f-6580-8d6a-f862ac4f62da@isovalent.com> (raw)
In-Reply-To: <CAEf4BzatvJORZvkz37_XJxvk5+Amr8V8iHq=1_4k_uCz0fE-eQ@mail.gmail.com>

2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> Replace the calls to deprecated function btf__get_from_id() with calls
>> to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
>> Update the surrounding code accordingly (instead of passing a pointer to
>> the btf struct, get it as a return value from the function). Also make
>> sure that btf__free() is called on the pointer after use.
>>
>> v2:
>> - Given that btf__load_from_kernel_by_id() has changed since v1, adapt
>>   the code accordingly instead of just renaming the function. Also add a
>>   few calls to btf__free() when necessary.
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>> ---
>>  tools/bpf/bpftool/btf.c                      |  8 ++----
>>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
>>  tools/bpf/bpftool/map.c                      | 16 +++++------
>>  tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
>>  tools/perf/util/bpf-event.c                  | 11 ++++----
>>  tools/perf/util/bpf_counter.c                | 12 ++++++--
>>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
>>  7 files changed, 51 insertions(+), 35 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index 09ae0381205b..12787758ce03 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
>>                 }
>>                 return btf_vmlinux;
>>         } else if (info->btf_value_type_id) {
>> -               int err;
>> -
>> -               err = btf__get_from_id(info->btf_id, &btf);
>> -               if (err || !btf) {
>> +               btf = btf__load_from_kernel_by_id(info->btf_id);
>> +               if (libbpf_get_error(btf)) {
>>                         p_err("failed to get btf");
>> -                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
>> +                       if (!btf)
>> +                               btf = ERR_PTR(-ESRCH);
> 
> why not do a simpler (less conditionals)
> 
> err = libbpf_get_error(btf);
> if (err) {
>     btf = ERR_PTR(err);
> }
> 
> ?

Because if btf is NULL at this stage, this would change the return value
from -ESRCH to NULL. This would be problematic in mapdump(), since we
check this value ("if (IS_ERR(btf))") to detect a failure in
get_map_kv_btf().

I could change that check in mapdump() to use libbpf_get_error()
instead, but in that case it would similarly change the return value for
mapdump() (and errno), which I think would be propagated up to main()
and would return 0 instead of -ESRCH. This does not seem suitable and
would play badly with batch mode, among other things.

So I'm considering keeping the one additional if.

> 
>>                 }
>>         }
>>
>> @@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>>                             void *value)
>>  {
>>         json_writer_t *btf_wtr;
>> -       struct btf *btf = NULL;
>> -       int err;
>> +       struct btf *btf;
>>
>> -       err = btf__get_from_id(info->btf_id, &btf);
>> -       if (err) {
>> +       btf = btf__load_from_kernel_by_id(info->btf_id);
>> +       if (libbpf_get_error(btf)) {
>>                 p_err("failed to get btf");
>>                 return;
>>         }
> 
> [...]
> 
>>
>>         func_info = u64_to_ptr(info->func_info);
>> @@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>>                 kernel_syms_destroy(&dd);
>>         }
>>
>> +       btf__free(btf);
>> +
> 
> warrants a Fixes: tag?

I don't mind adding the tags, but do they have any advantage here? My
understanding is that they tend to be neon signs for backports to stable
branches, but this patch depends on btf__load_from_kernel_by_id(),
meaning more patches to pull. I'll see if I can move the btf__free()
fixes to a separate commit, maybe.

  reply	other threads:[~2021-07-23  9:52 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21 15:38 [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF Quentin Monnet
2021-07-21 15:38 ` [PATCH bpf-next v2 1/5] libbpf: rename btf__load() as btf__load_into_kernel() Quentin Monnet
2021-07-21 15:38 ` [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id() Quentin Monnet
2021-07-23  0:39   ` Andrii Nakryiko
2021-07-23  9:31     ` Quentin Monnet
2021-07-23 15:54       ` Andrii Nakryiko
2021-07-23 16:13         ` Quentin Monnet
2021-07-23 17:18           ` Andrii Nakryiko
2021-07-23 17:44             ` Quentin Monnet
2021-07-21 15:38 ` [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id() Quentin Monnet
2021-07-23  0:48   ` Andrii Nakryiko
2021-07-23  9:52     ` Quentin Monnet [this message]
2021-07-23 15:57       ` Andrii Nakryiko
2021-07-23 16:17         ` Quentin Monnet
2021-07-21 15:38 ` [PATCH bpf-next v2 4/5] libbpf: add split BTF support for btf__load_from_kernel_by_id() Quentin Monnet
2021-07-23  0:49   ` Andrii Nakryiko
2021-07-21 15:38 ` [PATCH bpf-next v2 5/5] tools: bpftool: support dumping split BTF by id Quentin Monnet
2021-07-23  0:58 ` [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF Andrii Nakryiko
2021-07-23  2:45   ` Andrii Nakryiko
2021-07-23  9:58     ` Quentin Monnet
2021-07-23 15:51       ` Andrii Nakryiko
2021-07-27 11:39         ` Quentin Monnet
2021-07-27 20:49           ` Andrii Nakryiko
2021-07-28 21:54             ` Quentin Monnet
2021-07-28 22:29               ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3802e42d-321f-6580-8d6a-f862ac4f62da@isovalent.com \
    --to=quentin@isovalent.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=john.fastabend@gmail.com \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).