Date:   Tue, 27 Jul 2021 13:49:34 -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 v2 0/5] libbpf: rename btf__get_from_id() and
 btf__load() APIs, support split BTF

On Tue, Jul 27, 2021 at 4:39 AM Quentin Monnet <quentin@...valent.com> wrote:
>
> 2021-07-23 08:51 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> > On Fri, Jul 23, 2021 at 2:58 AM Quentin Monnet <quentin@...valent.com> wrote:
> >>
> >> 2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> >>> On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko
> >>> <andrii.nakryiko@...il.com> wrote:
> >>>>
> >>>> On Wed, Jul 21, 2021 at 8:38 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 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,

Powered by blists - more mailing lists