| Message ID | 20210721153808.6902-1-quentin@isovalent.com |
|---|---|
| Headers | show |
| Series | libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF | expand |
On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.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 not removed or marked as deprecated yet, there > should be in a future libbpf version. Oh, and I was thinking about this whole deprecation having to be done in two steps. It's super annoying to keep track of that. Ideally, we'd have some macro that can mark API deprecated "in the future", when actual libbpf version is >= to defined version. So something like this: LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6") We'd need to make sure that during the build time we have some LIBBPF_VERSION macro available against which we compare the expected version and add or not the __attribute__((deprecated)). Does this make sense? WDYT? I haven't looked into how hard it would be to implement this, but it should be easy enough, so if you'd like some macro challenge, please take a stab at it. Having this it would be possible to make all the deprecations at the same time that we add replacement APIs and not ask anyone to follow-up potentially a month or two later, right? > > 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 > > v2: > - Remove deprecation marking of legacy functions (patch 4/6 from v1). > - Make btf__load_from_kernel_by_id{,_split}() return the btf struct. > - Add new functions to v0.5.0 API (and not v0.6.0). > > Quentin Monnet (5): > libbpf: rename btf__load() as btf__load_into_kernel() > libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id() > tools: replace btf__get_from_id() with btf__load_from_kernel_by_id() > 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 | 16 +++++----- > tools/bpf/bpftool/prog.c | 29 +++++++++++------ > tools/lib/bpf/btf.c | 33 ++++++++++++++------ > tools/lib/bpf/btf.h | 4 +++ > tools/lib/bpf/libbpf.c | 7 +++-- > tools/lib/bpf/libbpf.map | 3 ++ > tools/perf/util/bpf-event.c | 11 ++++--- > tools/perf/util/bpf_counter.c | 12 +++++-- > tools/testing/selftests/bpf/prog_tests/btf.c | 4 ++- > 11 files changed, 86 insertions(+), 47 deletions(-) > > -- > 2.30.2 >
On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.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 not removed or marked as deprecated yet, there > > should be in a future libbpf version. > > Oh, and I was thinking about this whole deprecation having to be done > in two steps. It's super annoying to keep track of that. Ideally, we'd > have some macro that can mark API deprecated "in the future", when > actual libbpf version is >= to defined version. So something like > this: > > LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6") Better: LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6") > > > We'd need to make sure that during the build time we have some > LIBBPF_VERSION macro available against which we compare the expected > version and add or not the __attribute__((deprecated)). > > Does this make sense? WDYT? I haven't looked into how hard it would be > to implement this, but it should be easy enough, so if you'd like some > macro challenge, please take a stab at it. > > Having this it would be possible to make all the deprecations at the > same time that we add replacement APIs and not ask anyone to follow-up > potentially a month or two later, right? > > > > > 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 > > > > v2: > > - Remove deprecation marking of legacy functions (patch 4/6 from v1). > > - Make btf__load_from_kernel_by_id{,_split}() return the btf struct. > > - Add new functions to v0.5.0 API (and not v0.6.0). > > > > Quentin Monnet (5): > > libbpf: rename btf__load() as btf__load_into_kernel() > > libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id() > > tools: replace btf__get_from_id() with btf__load_from_kernel_by_id() > > 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 | 16 +++++----- > > tools/bpf/bpftool/prog.c | 29 +++++++++++------ > > tools/lib/bpf/btf.c | 33 ++++++++++++++------ > > tools/lib/bpf/btf.h | 4 +++ > > tools/lib/bpf/libbpf.c | 7 +++-- > > tools/lib/bpf/libbpf.map | 3 ++ > > tools/perf/util/bpf-event.c | 11 ++++--- > > tools/perf/util/bpf_counter.c | 12 +++++-- > > tools/testing/selftests/bpf/prog_tests/btf.c | 4 ++- > > 11 files changed, 86 insertions(+), 47 deletions(-) > > > > -- > > 2.30.2 > >
2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> >> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.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 not removed or marked as deprecated yet, there >>> should be in a future libbpf version. >> >> Oh, and I was thinking about this whole deprecation having to be done >> in two steps. It's super annoying to keep track of that. Ideally, we'd >> have some macro that can mark API deprecated "in the future", when >> actual libbpf version is >= to defined version. So something like >> this: >> >> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6") > > Better: > > LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6") I was considering a very advanced feature called “opening a new GitHub issue” to track this :). But the macro game sounds interesting, I'll look into it for next version. One nit with LIBBPF_DEPRECATED_SINCE() is that the warning mentions a version (here v0.6) that we are unsure will exist (say we jump from v0.5 to v1.0). But I don't suppose that's a real issue. Thanks for the feedback! Quentin
On Fri, Jul 23, 2021 at 2:58 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > >> > >> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.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 not removed or marked as deprecated yet, there > >>> should be in a future libbpf version. > >> > >> Oh, and I was thinking about this whole deprecation having to be done > >> in two steps. It's super annoying to keep track of that. Ideally, we'd > >> have some macro that can mark API deprecated "in the future", when > >> actual libbpf version is >= to defined version. So something like > >> this: > >> > >> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6") > > > > Better: > > > > LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6") > > I was considering a very advanced feature called “opening a new GitHub Someone gotta track and ping people at the right time even with issues, so yeah, it's suboptimal. > issue” to track this :). But the macro game sounds interesting, I'll > look into it for next version. > > One nit with LIBBPF_DEPRECATED_SINCE() is that the warning mentions a > version (here v0.6) that we are unsure will exist (say we jump from v0.5 > to v1.0). But I don't suppose that's a real issue. There will always be a +0.1 version just to get deprecation activated. This is for the reason I explained: we add replacement API in 0.X, but can mark deprecated API in 0.(X+1), so we won't skip it, even if we have to wait 2 extra months before 1.0. So I wouldn't worry about this. > > Thanks for the feedback! > Quentin
2021-07-23 08:51 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Fri, Jul 23, 2021 at 2:58 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> 2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> >>> On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko >>> <andrii.nakryiko@gmail.com> wrote: >>>> >>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.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 not removed or marked as deprecated yet, there >>>>> should be in a future libbpf version. >>>> >>>> Oh, and I was thinking about this whole deprecation having to be done >>>> in two steps. It's super annoying to keep track of that. Ideally, we'd >>>> have some macro that can mark API deprecated "in the future", when >>>> actual libbpf version is >= to defined version. So something like >>>> this: >>>> >>>> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6") >>> >>> Better: >>> >>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6") So I've been looking into this, and it's not _that_ simple to do. Unless I missed something about preprocessing macros, I cannot bake a "#if" in a "#define", to have the attribute printed if and only if the current version is >= 0.6 in this example. I've come up with something, but it is not optimal because I have to write a check and macros for each version number used with the LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part I guess we could generate a header with those macros from the Makefile and include it in libbpf_common.h, but that does not really look much cleaner to me. Here's my current code, below - does it correspond to what you had in mind? Or did you think of something else? ------ diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index ec14aa725bb0..095d5dc30d50 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \ grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \ sort -rV | head -n1 | cut -d'_' -f2) LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION))) +LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION)))) MAKEFLAGS += --no-print-directory @@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall override CFLAGS += $(INCLUDES) override CFLAGS += -fvisibility=hidden override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 +override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION) +override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION) # flags specific for shared library SHLIB_FLAGS := -DSHARED -fPIC diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index cf8490f95641..8b6b5442dbd8 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path); LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf); LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf); -LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); +LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") +int btf__get_from_id(__u32 id, struct btf **btf); LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf); LIBBPF_API int btf__load(struct btf *btf); diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h index 947d8bd8a7bb..9ba9f8135dc8 100644 --- a/tools/lib/bpf/libbpf_common.h +++ b/tools/lib/bpf/libbpf_common.h @@ -17,6 +17,28 @@ #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg))) +#ifndef LIBBPF_DEPRECATED_SINCE +#define __LIBBPF_VERSION_CHECK(major, minor) \ + LIBBPF_MAJOR_VERSION > major || \ + (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor) + +/* Add checks for other versions below when planning deprecation of API symbols + * with the LIBBPF_DEPRECATED_SINCE macro. + */ +#if __LIBBPF_VERSION_CHECK(0, 6) +#define __LIBBPF_MARK_DEPRECATED_0_6(X) X +#else +#define __LIBBPF_MARK_DEPRECATED_0_6(X) +#endif + +#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ + __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg)) + +/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */ +#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ + __LIBBPF_DEPRECATED_SINCE(major, minor, msg) +#endif /* LIBBPF_DEPRECATED_SINCE */ + /* Helper macro to declare and initialize libbpf options struct * * This dance with uninitialized declaration, followed by memset to zero,
On Tue, Jul 27, 2021 at 4:39 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2021-07-23 08:51 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Fri, Jul 23, 2021 at 2:58 AM Quentin Monnet <quentin@isovalent.com> wrote: > >> > >> 2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > >>> On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko > >>> <andrii.nakryiko@gmail.com> wrote: > >>>> > >>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.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 not removed or marked as deprecated yet, there > >>>>> should be in a future libbpf version. > >>>> > >>>> Oh, and I was thinking about this whole deprecation having to be done > >>>> in two steps. It's super annoying to keep track of that. Ideally, we'd > >>>> have some macro that can mark API deprecated "in the future", when > >>>> actual libbpf version is >= to defined version. So something like > >>>> this: > >>>> > >>>> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6") > >>> > >>> Better: > >>> > >>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6") > > So I've been looking into this, and it's not _that_ simple to do. Unless > I missed something about preprocessing macros, I cannot bake a "#if" in > a "#define", to have the attribute printed if and only if the current > version is >= 0.6 in this example. > > I've come up with something, but it is not optimal because I have to > write a check and macros for each version number used with the > LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part > I guess we could generate a header with those macros from the Makefile > and include it in libbpf_common.h, but that does not really look much > cleaner to me. Yeah, let's not add unnecessary code generation. It sucks, of course, that we can't do #ifdef inside a macro :( So it's either do something like what you did with defining version-specific macros, which is actually not too bad, because it's not like we have tons of those versions anyways. LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); Alternatively, we can go with: #if LIBBPF_AT_OR_NEWER(0, 6) LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead") #endif LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf); I don't really dislike the second variant too much either, but LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some comments below about implementation. > > Here's my current code, below - does it correspond to what you had in > mind? Or did you think of something else? > > ------ > > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile > index ec14aa725bb0..095d5dc30d50 100644 > --- a/tools/lib/bpf/Makefile > +++ b/tools/lib/bpf/Makefile > @@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \ > grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \ > sort -rV | head -n1 | cut -d'_' -f2) > LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION))) > +LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION)))) Given all this is for internal use, I'd instead define something like __LIBBPF_CURVER as an integer that is easy to compare against: #define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 + LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION That will simplify some stuff below and is generally easier to use in code, if we will need this somewhere to use explicitly. > > MAKEFLAGS += --no-print-directory > > @@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall > override CFLAGS += $(INCLUDES) > override CFLAGS += -fvisibility=hidden > override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 > +override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION) > +override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION) > > # flags specific for shared library > SHLIB_FLAGS := -DSHARED -fPIC > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > index cf8490f95641..8b6b5442dbd8 100644 > --- a/tools/lib/bpf/btf.h > +++ b/tools/lib/bpf/btf.h > @@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path); > LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf); > LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); > LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf); > -LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); > +LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") nit: given how long those deprecations will be, let's keep them at a separate (first) line and keep LIBBPF_API near the function declaration itself > +int btf__get_from_id(__u32 id, struct btf **btf); > > LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf); > LIBBPF_API int btf__load(struct btf *btf); > diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h > index 947d8bd8a7bb..9ba9f8135dc8 100644 > --- a/tools/lib/bpf/libbpf_common.h > +++ b/tools/lib/bpf/libbpf_common.h > @@ -17,6 +17,28 @@ > > #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg))) > > +#ifndef LIBBPF_DEPRECATED_SINCE why #ifndef conditional? > +#define __LIBBPF_VERSION_CHECK(major, minor) \ > + LIBBPF_MAJOR_VERSION > major || \ > + (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor) so we don't need this if we do __LIBBPF_CURVER > + > +/* Add checks for other versions below when planning deprecation of API symbols > + * with the LIBBPF_DEPRECATED_SINCE macro. > + */ > +#if __LIBBPF_VERSION_CHECK(0, 6) > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) X > +#else > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) > +#endif > + > +#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ > + __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg)) > + > +/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */ > +#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ > + __LIBBPF_DEPRECATED_SINCE(major, minor, msg) Is it needed for some macro value concatenation magic to have this nested __LIBBPF_DEPRECATED_SINCE? > +#endif /* LIBBPF_DEPRECATED_SINCE */ > + > /* Helper macro to declare and initialize libbpf options struct > * > * This dance with uninitialized declaration, followed by memset to zero,
On Tue, 27 Jul 2021 at 21:49, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > >>> > > >>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6") > > > > So I've been looking into this, and it's not _that_ simple to do. Unless > > I missed something about preprocessing macros, I cannot bake a "#if" in > > a "#define", to have the attribute printed if and only if the current > > version is >= 0.6 in this example. > > > > I've come up with something, but it is not optimal because I have to > > write a check and macros for each version number used with the > > LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part > > I guess we could generate a header with those macros from the Makefile > > and include it in libbpf_common.h, but that does not really look much > > cleaner to me. > > Yeah, let's not add unnecessary code generation. It sucks, of course, > that we can't do #ifdef inside a macro :( > > So it's either do something like what you did with defining > version-specific macros, which is actually not too bad, because it's > not like we have tons of those versions anyways. > > LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") > LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); > > Alternatively, we can go with: > > #if LIBBPF_AT_OR_NEWER(0, 6) > LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead") > #endif > LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf); > > I don't really dislike the second variant too much either, but > LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some > comments below about implementation. Ok. > > > > > Here's my current code, below - does it correspond to what you had in > > mind? Or did you think of something else? > > > > ------ > > > > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile > > index ec14aa725bb0..095d5dc30d50 100644 > > --- a/tools/lib/bpf/Makefile > > +++ b/tools/lib/bpf/Makefile > > @@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \ > > grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \ > > sort -rV | head -n1 | cut -d'_' -f2) > > LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION))) > > +LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION)))) > > Given all this is for internal use, I'd instead define something like > __LIBBPF_CURVER as an integer that is easy to compare against: > > #define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 + > LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION > > That will simplify some stuff below and is generally easier to use in > code, if we will need this somewhere to use explicitly. Did you mean computing __LIBBPF_CURVER in the Makefile, or in the header? I can do that if you want, although I'm not convinced it will simplify much. Instead of having one long-ish condition, we'll have to compute the integer for the current version, as well as for each of the versions that we list for deprecating functions. I suppose I can add another dedicated macro. Do you actually want the patch version? I chose to leave it aside because 1) I thought it would not be relevant for deprecating symbols, and 2) if anything like a -rc1 suffix is ever appended to the version, it makes it more complex to parse from the version string. > > > > > MAKEFLAGS += --no-print-directory > > > > @@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall > > override CFLAGS += $(INCLUDES) > > override CFLAGS += -fvisibility=hidden > > override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 > > +override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION) > > +override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION) > > > > # flags specific for shared library > > SHLIB_FLAGS := -DSHARED -fPIC > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > > index cf8490f95641..8b6b5442dbd8 100644 > > --- a/tools/lib/bpf/btf.h > > +++ b/tools/lib/bpf/btf.h > > @@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path); > > LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf); > > LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); > > LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf); > > -LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); > > +LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") > > nit: given how long those deprecations will be, let's keep them at a > separate (first) line and keep LIBBPF_API near the function > declaration itself I thought having the LIBBPF_API on a separate line would slightly reduce the risk, when moving lines around, to move the function prototype but not the deprecation attribute. But ok, fine. > > > +int btf__get_from_id(__u32 id, struct btf **btf); > > > > LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf); > > LIBBPF_API int btf__load(struct btf *btf); > > diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h > > index 947d8bd8a7bb..9ba9f8135dc8 100644 > > --- a/tools/lib/bpf/libbpf_common.h > > +++ b/tools/lib/bpf/libbpf_common.h > > @@ -17,6 +17,28 @@ > > > > #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg))) > > > > +#ifndef LIBBPF_DEPRECATED_SINCE > > why #ifndef conditional? Right, we don't expect to have the macro defined elsewhere. I'll remove it. > > > +#define __LIBBPF_VERSION_CHECK(major, minor) \ > > + LIBBPF_MAJOR_VERSION > major || \ > > + (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor) > > so we don't need this if we do __LIBBPF_CURVER Right, but we do need to compute an integer for each of the versions listed below (0.6 for now). I'll see if I can come up with something short. > > > + > > +/* Add checks for other versions below when planning deprecation of API symbols > > + * with the LIBBPF_DEPRECATED_SINCE macro. > > + */ > > +#if __LIBBPF_VERSION_CHECK(0, 6) > > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) X > > +#else > > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) > > +#endif > > + > > +#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ > > + __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg)) > > + > > +/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */ > > +#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ > > + __LIBBPF_DEPRECATED_SINCE(major, minor, msg) > > Is it needed for some macro value concatenation magic to have this > nested __LIBBPF_DEPRECATED_SINCE? I double-checked (I needed to, anyway), and it seems not. It's a leftover from an earlier version of my code, I'll clean it up before the proper submission. Thanks! Quentin
On Wed, Jul 28, 2021 at 2:54 PM Quentin Monnet <quentin@isovalent.com> wrote: > > On Tue, 27 Jul 2021 at 21:49, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > >>> > > > >>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6") > > > > > > So I've been looking into this, and it's not _that_ simple to do. Unless > > > I missed something about preprocessing macros, I cannot bake a "#if" in > > > a "#define", to have the attribute printed if and only if the current > > > version is >= 0.6 in this example. > > > > > > I've come up with something, but it is not optimal because I have to > > > write a check and macros for each version number used with the > > > LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part > > > I guess we could generate a header with those macros from the Makefile > > > and include it in libbpf_common.h, but that does not really look much > > > cleaner to me. > > > > Yeah, let's not add unnecessary code generation. It sucks, of course, > > that we can't do #ifdef inside a macro :( > > > > So it's either do something like what you did with defining > > version-specific macros, which is actually not too bad, because it's > > not like we have tons of those versions anyways. > > > > LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") > > LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); > > > > Alternatively, we can go with: > > > > #if LIBBPF_AT_OR_NEWER(0, 6) > > LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead") > > #endif > > LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf); > > > > I don't really dislike the second variant too much either, but > > LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some > > comments below about implementation. > > Ok. > > > > > > > > > Here's my current code, below - does it correspond to what you had in > > > mind? Or did you think of something else? > > > > > > ------ > > > > > > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile > > > index ec14aa725bb0..095d5dc30d50 100644 > > > --- a/tools/lib/bpf/Makefile > > > +++ b/tools/lib/bpf/Makefile > > > @@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \ > > > grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \ > > > sort -rV | head -n1 | cut -d'_' -f2) > > > LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION))) > > > +LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION)))) > > > > Given all this is for internal use, I'd instead define something like > > __LIBBPF_CURVER as an integer that is easy to compare against: > > > > #define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 + > > LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION > > > > That will simplify some stuff below and is generally easier to use in > > code, if we will need this somewhere to use explicitly. > > Did you mean computing __LIBBPF_CURVER in the Makefile, or in the > header? I was thinking Makefile, but if it's simpler to do in the header that's fine as well. > > I can do that if you want, although I'm not convinced it will simplify > much. Instead of having one long-ish condition, we'll have to compute > the integer for the current version, as well as for each of the versions > that we list for deprecating functions. I suppose I can add another > dedicated macro. feels like if we need to do some comparisons, then writing #if __LIBBPF_VER > 102 /* do something */ #endif is much simpler than comparing MAJOR_VERSION and MINOR_VERSION separately. It's just that currently with 0 major version it might look a bit awkward right now, but that's temporary. > > Do you actually want the patch version? I chose to leave it aside > because 1) I thought it would not be relevant for deprecating symbols, > and 2) if anything like a -rc1 suffix is ever appended to the version, > it makes it more complex to parse from the version string. yeah, you are probably right. major and minor should be enough > > > > > > > > > MAKEFLAGS += --no-print-directory > > > > > > @@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall > > > override CFLAGS += $(INCLUDES) > > > override CFLAGS += -fvisibility=hidden > > > override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 > > > +override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION) > > > +override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION) > > > > > > # flags specific for shared library > > > SHLIB_FLAGS := -DSHARED -fPIC > > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > > > index cf8490f95641..8b6b5442dbd8 100644 > > > --- a/tools/lib/bpf/btf.h > > > +++ b/tools/lib/bpf/btf.h > > > @@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path); > > > LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf); > > > LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); > > > LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf); > > > -LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); > > > +LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") > > > > nit: given how long those deprecations will be, let's keep them at a > > separate (first) line and keep LIBBPF_API near the function > > declaration itself > > I thought having the LIBBPF_API on a separate line would slightly reduce > the risk, when moving lines around, to move the function prototype but > not the deprecation attribute. But ok, fine. highly improbable and then we'll most probably catch it during build anyways > > > > > > +int btf__get_from_id(__u32 id, struct btf **btf); > > > > > > LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf); > > > LIBBPF_API int btf__load(struct btf *btf); > > > diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h > > > index 947d8bd8a7bb..9ba9f8135dc8 100644 > > > --- a/tools/lib/bpf/libbpf_common.h > > > +++ b/tools/lib/bpf/libbpf_common.h > > > @@ -17,6 +17,28 @@ > > > > > > #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg))) > > > > > > +#ifndef LIBBPF_DEPRECATED_SINCE > > > > why #ifndef conditional? > > Right, we don't expect to have the macro defined elsewhere. I'll remove > it. > > > > > > +#define __LIBBPF_VERSION_CHECK(major, minor) \ > > > + LIBBPF_MAJOR_VERSION > major || \ > > > + (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor) > > > > so we don't need this if we do __LIBBPF_CURVER > > Right, but we do need to compute an integer for each of the versions > listed below (0.6 for now). I'll see if I can come up with something > short. see above, I'd just do 102 etc. I wonder if 006 will be treated as an octal number, in that case probably fine to do just 6. Or we can have a small macro for this, of course. Don't know, doesn't seem to matter all that much > > > > > > + > > > +/* Add checks for other versions below when planning deprecation of API symbols > > > + * with the LIBBPF_DEPRECATED_SINCE macro. > > > + */ > > > +#if __LIBBPF_VERSION_CHECK(0, 6) > > > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) X > > > +#else > > > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) > > > +#endif > > > + > > > +#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ > > > + __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg)) > > > + > > > +/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */ > > > +#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \ > > > + __LIBBPF_DEPRECATED_SINCE(major, minor, msg) > > > > Is it needed for some macro value concatenation magic to have this > > nested __LIBBPF_DEPRECATED_SINCE? > > I double-checked (I needed to, anyway), and it seems not. It's a > leftover from an earlier version of my code, I'll clean it up before > the proper submission. ok, thanks > > Thanks! > Quentin
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 not removed or marked as deprecated yet, there should be in a future libbpf version. 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 v2: - Remove deprecation marking of legacy functions (patch 4/6 from v1). - Make btf__load_from_kernel_by_id{,_split}() return the btf struct. - Add new functions to v0.5.0 API (and not v0.6.0). Quentin Monnet (5): libbpf: rename btf__load() as btf__load_into_kernel() libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id() tools: replace btf__get_from_id() with btf__load_from_kernel_by_id() 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 | 16 +++++----- tools/bpf/bpftool/prog.c | 29 +++++++++++------ tools/lib/bpf/btf.c | 33 ++++++++++++++------ tools/lib/bpf/btf.h | 4 +++ tools/lib/bpf/libbpf.c | 7 +++-- tools/lib/bpf/libbpf.map | 3 ++ tools/perf/util/bpf-event.c | 11 ++++--- tools/perf/util/bpf_counter.c | 12 +++++-- tools/testing/selftests/bpf/prog_tests/btf.c | 4 ++- 11 files changed, 86 insertions(+), 47 deletions(-)