Message-ID: <20181203022704.GA31554@rdna-mbp.dhcp.thefacebook.com>
Date:   Mon, 3 Dec 2018 02:27:55 +0000
From:   Andrey Ignatov <rdna@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>, Yonghong Song <yhs@...com>,
        Martin Lau <kafai@...com>, Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 0/4] libbpf: ABI versioning and documentation

Daniel Borkmann <daniel@...earbox.net> [Tue, 2018-11-27 03:36 -0800]:
> On 11/27/2018 04:06 AM, Alexei Starovoitov wrote:
> > On Fri, Nov 23, 2018 at 04:44:31PM -0800, Andrey Ignatov wrote:
> >> This patch set adds ABI versioning and documentation to libbpf.
> >>
> >> Patch 1 renames btf_get_from_id to btf__get_from_id to follow naming
> >> convention.
> >> Patch 2 adds version script and has more details on ABI versioning.
> >> Patch 3 adds simple check that all global symbols are versioned.
> >> Patch 4 documents a few aspects of libbpf API and ABI in dev process.
> >>
> >> v1->v2:
> >> * add patch from Martin KaFai Lau <kafai@...com> to rename btf_get_from_id;
> >> * add documentation for libbpf API and ABI.
> > 
> > All looks great to me.
> > Thank you for adding the doc.
> > Applied to bpf-next.
> > 
> > We need to discuss the release model and version bumps.
> > For example I don't think it's necessary to bump the version
> > and update libbpf.map every time the new function is added.
> > I mean we can synchronize release of libbpf with the release of the kernel
> > or release it every N weeks.
> 
> +1, synchronizing with release of the kernel seems a natural fit
> given it's part of the kernel tree. Every N weeks might not even
> work since changes are not in Linus' tree at that point, I think
> this would probably just lead to confusion.

Sorry for delay folks, didn't have a chance to answer earlier.

After some thinking I agree that bumping ABI version every time, a new
symbol is being added, is probably overkill.

Though I think all new symbols added in a new release should go to
corresponding new ABI version, i.e. during release preparation we may
have a version X and all new symbols go to this version X. Once release
happens and X is released, no new symbols should be added to X and
instead a new X + 1 ABI version should be used.

If we simply add new symbols to existing ABI version (without
coordinating it with release) I can imaging a problem when application
is build and dynamically linked with libbpf on one host B, but run on a
different host R, both hosts have exactly same ABI version X of libbpf
DSO, but libbpf on B is newer and has some symbols (used by application)
that are not available in libbpf on R. In this scenario when application
is started on host R, dynamic linker will get DT_VERNEED from it,
identify that libbpf of version X is needed, will find that libbpf on
the host has this same version in DT_VERDEFNUM and will assume that all
symbols of that version are available, but will actually fail later on
symbol lookup for any "new" symbol.

It seems we all agree that ABI version bump should be synchronized with
some release.

> > So if we add new api functions during this release we can simply
> > add them to libbpf.map while keeping the version as 0.0.1

Yeah, "during" is very important IMO, but not after release happened.


> > I'd also consider the first 0.0.1 release to be experimental
> > while we're figuring out the right process.
> > For the next kernel/libbpf release I propose to bump it to 1.0.0
> > 
> > Another idea is to version it just like kernel and make libbpf version
> > always equal kernel version.
> > But I think that would be an overkill. libbpf isn't tightly coupled to
> > the kernel. Like we just merged the patch (prog_name/map_name probing
> > that allows new libbpf to work with older kernel.
> 
> Right, though we might end up bumping version each kernel release
> anyway. This would allow for more automation on that part if we
> could simply say that version looks like libbpf.so.4.20 or
> libbpf.so.4.21, etc, without us forgetting to send an extra patch

I think there is a confusion here. libbpf.so.4.20 and libbpf.so.4.21 are
two different file names, what usually means different SONAME. Renaming
a library this way is another way of versioning, yeah, but it's the
coarsest grained one that has many drawbacks and is basically the last
resort when other methods don't work.

ABI versioning added in this patch series is the way to avoid file
renaming and provide fine grained control over symbol versioning
instead.

That's being said the DSO file name should not be changed every time we
bump ABI version in the version script.

> every release just to bump version. Whether it's version -0.0.1,
> -1.0.0, -4.20 or say -2018.11.27 is probably mostly matter of
> preference but for the former two we would have to additionally
> establish some convention / process on when to bump which part
> of the three version component. I'd rather prefer if we could
> have kernel-like convention where a bump in major version is "just"
> yet another release but has no other special meaning attached to
> it in which case we could just adapt the same numbering scheme.

I don't have strong opinion on how specifically the version string
should look like.

One idea I have though is about "current" version that is not released
yet and where new symbols may be added. What about naming it
EXPERIMENTAL or something like this? This way we can provide a clear
contract that any experimental symbol can break before it's released
what gives us a room to fix recently added symbols if needed. Once it's
decided to make a new release all new symbols that are stable enough can
be moved from EXPERIMENTAL to a new stable ABI version and kept
compatible. That adds maintenance burden though.

Also we're discussing mostly adding new symbols what is compatible ABI
change. On the other hand if an existing symbol is being changed in an
incompatible way, we have no choice and should bump ABI version, making
the new semantic available in new version, keeping the old semantic in
original version and setting what version of the symbol will be used by
default.  That's the whole other story that we can probably figure out
the first time we have such ABI change.


-- 
Andrey Ignatov

Powered by blists - more mailing lists