Message-ID: <CAEf4BzYzJuPt8Fct2pOTPjHLiiyGPQw05rFNK4d+MAJTC_itkw@mail.gmail.com>
Date:   Mon, 7 Dec 2020 22:38:44 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH bpf] tools/bpftool: Add/Fix support for modules btf dump

On Mon, Dec 7, 2020 at 10:26 PM Saeed Mahameed <saeed@...nel.org> wrote:
>
> On Mon, 2020-12-07 at 19:14 -0800, Andrii Nakryiko wrote:
> > On Sun, Dec 6, 2020 at 9:21 PM <saeed@...nel.org> wrote:
> > > From: Saeed Mahameed <saeedm@...dia.com>
> > >
> > > While playing with BTF for modules, i noticed that executing the
> > > command:
> > > $ bpftool btf dump id <module's btf id>
> > >
> > > Fails due to lack of information in the BTF data.
> > >
> > > Maybe I am missing a step but actually adding the support for this
> > > is
> > > very simple.
> >
> > yes, bpftool takes -B <path> argument for specifying base BTF. So if
> > you added -B /sys/kernel/btf/vmlinux, it should have worked. I've
> > added auto-detection logic for the case of `btf dump file
> > /sys/kernel/btf/<module>` (see [0]), and we can also add it for when
> > ID corresponds to a module BTF. But I think it's simplest to re-use
> > the logic and just open /sys/kernel/btf/vmlinux, instead of adding
> > narrowly-focused libbpf API for that.
> >
>
> When dumping with a file it works even without the -B since you lookup
> the vmlinux file, but the issue is not dumping from a file source, we
> need it by id..
>
> > > To completely parse modules BTF data, we need the vmlinux BTF as
> > > their
> > > "base btf", which can be easily found by iterating through the btf
> > > ids and
> > > looking for btf.name == "vmlinux".
> > >
> > > I am not sure why this hasn't been added by the original patchset
> >
> > because I never though of dumping module BTF by id, given there is
> > nicely named /sys/kernel/btf/<module> :)
> >
>
> What if i didn't compile my kernel with SYSFS ? a user experience is a
> user experience, there is no reason to not support dump a module btf by
> id or to have different behavior for different BTF sources.

Hm... I didn't claim otherwise and didn't oppose the feature, why the
lecture about user experience?

Not having sysfs is a valid point. In such cases, if BTF dumping is
from ID and we see that it's a module BTF, finding vmlinux BTF from ID
makes sense.

>
> I can revise this patch to support -B option and lookup vmlinux file if
> not provided for module btf dump by ids.

yep

>
> but we  still need to pass base_btf to btf__get_from_id() in order to
> support that, as was done for btf__parse_split() ... :/

btf__get_from_id_split() might be needed, yes.

>
> Are you sure you don't like the current patch/libbpf API ? it is pretty
> straight forward and correct.

I definitely don't like adding btf_get_kernel_id() API to libbpf.
There is nothing special about it to warrant adding it as a public
API. Everything we discussed can be done by bpftool.

>
> > > "Integrate kernel module BTF support", as adding the support for
> > > this is very trivial. Unless i am missing something, CCing Andrii..
> > >
> > > Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> > > CC: Andrii Nakryiko <andrii@...nel.org>
> > > ---
> > >  tools/lib/bpf/btf.c      | 57
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  tools/lib/bpf/btf.h      |  2 ++
> > >  tools/lib/bpf/libbpf.map |  1 +
> > >  3 files changed, 60 insertions(+)
> > >
> >
> > [...]
>

Powered by blists - more mailing lists