Message-ID: <CAEf4BzbjN+zjio3HPRkGLRgZpbLj9MUGLnXt1KDSsoOHB8_v3Q@mail.gmail.com>
Date:   Fri, 30 Jul 2021 10:24:38 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Quentin Monnet <quentin@...valent.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v3 0/8] libbpf: rename btf__get_from_id() and
 btf__load() APIs, support split BTF

On Fri, Jul 30, 2021 at 8:23 AM Quentin Monnet <quentin@...valent.com> wrote:
>
> 2021-07-29 17:31 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> > On Thu, Jul 29, 2021 at 9:20 AM Quentin Monnet <quentin@...valent.com> wrote:
> >>
> >> As part of the effort to move towards a v1.0 for libbpf [0], this set
> >> improves some confusing function names related to BTF loading from and to
> >> the kernel:
> >>
> >> - btf__load() becomes btf__load_into_kernel().
> >> - btf__get_from_id becomes btf__load_from_kernel_by_id().
> >> - A new version btf__load_from_kernel_by_id_split() extends the former to
> >>   add support for split BTF.
> >>
> >> The old functions are marked for deprecation for the next minor version
> >> (0.6) of libbpf.
> >>
> >> The last patch is a trivial change to bpftool to add support for dumping
> >> split BTF objects by referencing them by their id (and not only by their
> >> BTF path).
> >>
> >> [0] https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
> >>
> >> v3:
> >> - Use libbpf_err_ptr() in btf__load_from_kernel_by_id(), ERR_PTR() in
> >>   bpftool's get_map_kv_btf().
> >> - Move the definition of btf__load_from_kernel_by_id() closer to the
> >>   btf__parse() group in btf.h (move the legacy function with it).
> >> - Fix a bug on the return value in libbpf_find_prog_btf_id(), as a new
> >>   patch.
> >> - Move the btf__free() fixes to their own patch.
> >> - Add "Fixes:" tags to relevant patches.
> >> - Re-introduce deprecation (removed in v2) for the legacy functions, as a
> >>   new macro LIBBPF_DEPRECATED_SINCE(major, minor, message).
> >>
> >> v2:
> >> - Remove deprecation marking of legacy functions (patch 4/6 from v1).
> >> - Make btf__load_from_kernel_by_id{,_split}() return the btf struct, adjust
> >>   surrounding code and call btf__free() when missing.
> >> - Add new functions to v0.5.0 API (and not v0.6.0).
> >>
> >> Quentin Monnet (8):
> >>   libbpf: return non-null error on failures in libbpf_find_prog_btf_id()
> >>   libbpf: rename btf__load() as btf__load_into_kernel()
> >>   libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()
> >>   tools: free BTF objects at various locations
> >>   tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()
> >>   libbpf: prepare deprecation of btf__get_from_id(), btf__load()
> >>   libbpf: add split BTF support for btf__load_from_kernel_by_id()
> >>   tools: bpftool: support dumping split BTF by id
> >>
> >>  tools/bpf/bpftool/btf.c                      |  8 ++---
> >>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
> >>  tools/bpf/bpftool/map.c                      | 14 ++++-----
> >>  tools/bpf/bpftool/prog.c                     | 29 +++++++++++------
> >>  tools/lib/bpf/Makefile                       |  3 ++
> >>  tools/lib/bpf/btf.c                          | 33 ++++++++++++++------
> >>  tools/lib/bpf/btf.h                          |  7 ++++-
> >>  tools/lib/bpf/libbpf.c                       | 11 ++++---
> >>  tools/lib/bpf/libbpf.map                     |  3 ++
> >>  tools/lib/bpf/libbpf_common.h                | 19 +++++++++++
> >>  tools/perf/util/bpf-event.c                  | 11 ++++---
> >>  tools/perf/util/bpf_counter.c                | 12 +++++--
> >>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
> >>  13 files changed, 113 insertions(+), 47 deletions(-)
> >>
> >> --
> >> 2.30.2
> >>
> >
> > I dropped patch #7 with deprecations and LIBBPF_DEPRECATED_SINCE and
> > applied to bpf-next.
> >
> > Current LIBBPF_DEPRECATED_SINCE approach doesn't work (and you should
> > have caught this when you built selftests/bpf, what happened there?).
> > bpftool build generates warnings like this:
> >
> > In file included from /data/users/andriin/linux/tools/lib/bpf/libbpf.h:20,
> >                  from xlated_dumper.c:10:
> > /data/users/andriin/linux/tools/lib/bpf/libbpf_common.h:22:23:
> > warning: "LIBBPF_MAJOR_VERSION" is not defined, evaluates to 0
> > [-Wundef]
> >   __LIBBPF_GET_VERSION(LIBBPF_MAJOR_VERSION, LIBBPF_MINOR_VERSION)
> >                        ^~~~~~~~~~~~~~~~~~~~
>
> Apologies, I didn't realise the change would impact external applications.

It doesn't matter, we expect everyone to compile selftest (just `make`
in tools/testing/selftests/bpf) and run at least test_progs,
preferably also test_maps and test_verifier. Especially with vmtest.sh
script it's quite simple (once you get latest Clang and pahole
compiled locally). We obviously have CI and maintainers as the last
line of defense, but that should be the last line of defense, not the
main line :)

>
> >
> > And it makes total sense. LIBBPF_DEPRECATED_SINCE() assumes
> > LIBBPF_MAJOR_VERSION/LIBBPF_MINOR_VERSION is defined at compilation
> > time of the *application that is using libbpf*, not just libbpf's
> > compilation time. And that's clearly a bogus assumption which we can't
> > and shouldn't make. The right approach will be to define
> > LIBBPF_MAJOR_VERSION/LIBBPF_MINOR_VERSION in some sort of
> > auto-generated header, included from libbpf_common.h and installed as
> > part of libbpf package.
>
> So generating this header is easy. Installing it with the other headers
> is simple too. It becomes a bit trickier when we build outside of the
> directory (it seems I need to pass -I$(OUTPUT) to build libbpf).

Not sure why using the header is tricky. We auto-generate
bpf_helper_defs.h, which is included from bpf_helpers.h, which is
included in every single libbpf-using application. Works good with no
extra magic.

>
> The step I'm most struggling with at the moment is bpftool, which
> bootstraps a first version of itself before building libbpf, by looking
> at the headers directly in libbpf's directory. It means that the
> generated header with the version number has not yet been generated. Do
> you think it is worth changing bpftool's build steps to implement this
> deprecation helper?

If it doesn't do that already, bpftool should do `make install` for
libbpf, not just build. Install will put all the headers, generated or
otherwise, into a designated destination folder, which should be
passed as -I parameter. But that should be already happening due to
bpf_helper_defs.h.

>
> Alternatively, wouldn't it make more sense to have a script in the
> GitHub repo for libbpf, and to run it once during the release process of
> a new version to update, say, the version number, or even the
> deprecation status directly?

I'd like to avoid extra manual steps that I or someone else will
definitely forget from time to time. Again, taking bpf_helper_defs.h
as a precedent. In the kernel repo we auto-generate it during build.
But when we sync libbpf to Github, we copy and check-in
bpf_helper_defs.h, so it's always available there (and will get
installed on `make install` or during packaging). We should do the
same for this new header (libbpf_version.h?).

>
> >
> > Anyways, I've removed all the LIBBPF_DEPRECATED_SINCE stuff and
> > applied all the rest, as it looks good and is a useful addition.
>
> Thanks.
> Quentin

Powered by blists - more mailing lists