Message-ID: <1d3d53c7-d3cc-cd76-220c-c7b250111229@isovalent.com>
Date:   Wed, 9 Feb 2022 19:15:08 +0000
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.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 3/3] bpftool: Update versioning scheme, align
 on libbpf's version number

2022-02-09 09:53 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> On Wed, Feb 9, 2022 at 4:37 AM Quentin Monnet <quentin@...valent.com> wrote:
>>
>> 2022-02-08 16:39 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
>>> On Tue, Feb 8, 2022 at 4:07 AM Quentin Monnet <quentin@...valent.com> wrote:
>>>>
>>>> Since the notion of versions was introduced for bpftool, it has been
>>>> following the version number of the kernel (using the version number
>>>> corresponding to the tree in which bpftool's sources are located). The
>>>> rationale was that bpftool's features are loosely tied to BPF features
>>>> in the kernel, and that we could defer versioning to the kernel
>>>> repository itself.
>>>>
>>>> But this versioning scheme is confusing today, because a bpftool binary
>>>> should be able to work with both older and newer kernels, even if some
>>>> of its recent features won't be available on older systems. Furthermore,
>>>> if bpftool is ported to other systems in the future, keeping a
>>>> Linux-based version number is not a good option.
>>>>
>>>> Looking at other options, we could either have a totally independent
>>>> scheme for bpftool, or we could align it on libbpf's version number
>>>> (with an offset on the major version number, to avoid going backwards).
>>>> The latter comes with a few drawbacks:
>>>>
>>>> - We may want bpftool releases in-between two libbpf versions. We can
>>>>   always append pre-release numbers to distinguish versions, although
>>>>   those won't look as "official" as something with a proper release
>>>>   number. But at the same time, having bpftool with version numbers that
>>>>   look "official" hasn't really been an issue so far.
>>>>
>>>> - If no new feature lands in bpftool for some time, we may move from
>>>>   e.g. 6.7.0 to 6.8.0 when libbpf levels up and have two different
>>>>   versions which are in fact the same.
>>>>
>>>> - Following libbpf's versioning scheme sounds better than kernel's, but
>>>>   ultimately it doesn't make too much sense either, because even though
>>>>   bpftool uses the lib a lot, its behaviour is not that much conditioned
>>>>   by the internal evolution of the library (or by new APIs that it may
>>>>   not use).
>>>>
>>>> Having an independent versioning scheme solves the above, but at the
>>>> cost of heavier maintenance. Developers will likely forget to increase
>>>> the numbers when adding features or bug fixes, and we would take the
>>>> risk of having to send occasional "catch-up" patches just to update the
>>>> version number.
>>>>
>>>> Based on these considerations, this patch aligns bpftool's version
>>>> number on libbpf's. This is not a perfect solution, but 1) it's
>>>> certainly an improvement over the current scheme, 2) the issues raised
>>>> above are all minor at the moment, and 3) we can still move to an
>>>> independent scheme in the future if we realise we need it.
>>>>
>>>> Given that libbpf is currently at version 0.7.0, and bpftool, before
>>>> this patch, was at 5.16, we use an offset of 6 for the major version,
>>>> bumping bpftool to 6.7.0.
>>>>
>>>> It remains possible to manually override the version number by setting
>>>> BPFTOOL_VERSION when calling make.
>>>>
>>>> Signed-off-by: Quentin Monnet <quentin@...valent.com>
>>>> ---
>>>> Contrarily to the previous discussion and to what the first patch of the
>>>> set does, I chose not to use the libbpf_version_string() API from libbpf
>>>> to compute the version for bpftool. There were three reasons for that:
>>>>
>>>> - I don't feel comfortable having bpftool's version number computed at
>>>>   runtime. Somehow it really feels like we should now it when we compile
>>>
>>> Fair, but why not use LIBBPF_MAJOR_VERSION and LIBBPF_MINOR_VERSION to
>>> define BPFTOOL_VERSION (unless it's overridden).
>>
>> I forgot the macros were exposed, which is silly, because I was the one
>> to help expose them in the first place. Anyway.
>>
>>> Which all seems to be
>>> doable at compilation time in C code, not in Makefile. This will work
>>> with Github version of libbpf just as well with no extra Makefile
>>> changes (and in general, the less stuff is done in Makefile the
>>> better, IMO).
>>>
>>> Version string can also be "composed" at compile time with a bit of
>>> helper macro, see libbpf_version_string() implementation in libbpf.
>>
>> Sounds good, I can do that.

... Except that you can only compose so much. The preprocessor won't
allow me to sum libbpf's major version with the offset (6) before
turning it into a string. I need to think about this a bit more.

>>
>> This won't give me the patch number, though, only major and minor
>> version. We could add an additional LIBBPF_PATCH_VERSION to libbpf.
>> Although thinking about it, I'm not sure we need a patch version for
>> bpftool at the moment, because changes in libbpf's patch number is
>> unlikely to reflect any change in bpftool, so it makes little sense to
>> copy it. So I'm considering just leaving it at 0 in bpftool, and having
>> updates on major/minor numbers only when libbpf releases a major/minor
>> version. If we do want bugfix releases, it will still be possible to
>> overwrite the version number with BPFTOOL_VERSION anyway. What do you think?
> 
> So the patch version is not currently reflected in libbpf.map file. I
> do patch version bumps only when we detect some small issue after
> official release. It happened 2 or 3 times so far. I'm hesitant to
> expose that as LIBBPF_PATCH_VERSION, because I'll need to remember to
> bump it manually (and coordinating this between kernel sources and
> Github is a slow nightmare). Let's not rely on patch version, too much
> burden.

Agreed, thanks.
Quentin

Powered by blists - more mailing lists