Re: [PATCH bpf-next v3 0/8] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

2021-07-29 17:31 UTC-0700 ~ Andrii Nakryiko [off-list ref]

On Thu, Jul 29, 2021 at 9:20 AM Quentin Monnet [off-list ref] wrote:

quoted

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.

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

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?

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?

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