Re: [RFC bpf-next 0/5] Convert iproute2 to use libbpf (WIP)

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Wed, Aug 21, 2019 at 4:29 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>
>> > On Tue, Aug 20, 2019 at 01:47:01PM +0200, Toke Høiland-Jørgensen wrote:
>> >> iproute2 uses its own bpf loader to load eBPF programs, which has
>> >> evolved separately from libbpf. Since we are now standardising on
>> >> libbpf, this becomes a problem as iproute2 is slowly accumulating
>> >> feature incompatibilities with libbpf-based loaders. In particular,
>> >> iproute2 has its own (expanded) version of the map definition struct,
>> >> which makes it difficult to write programs that can be loaded with both
>> >> custom loaders and iproute2.
>> >>
>> >> This series seeks to address this by converting iproute2 to using libbpf
>> >> for all its bpf needs. This version is an early proof-of-concept RFC, to
>> >> get some feedback on whether people think this is the right direction.
>> >>
>> >> What this series does is the following:
>> >>
>> >> - Updates the libbpf map definition struct to match that of iproute2
>> >>   (patch 1).
>> >> - Adds functionality to libbpf to support automatic pinning of maps when
>> >>   loading an eBPF program, while re-using pinned maps if they already
>> >>   exist (patches 2-3).
>> >> - Modifies iproute2 to make it possible to compile it against libbpf
>> >>   without affecting any existing functionality (patch 4).
>> >> - Changes the iproute2 eBPF loader to use libbpf for loading XDP
>> >>   programs (patch 5).
>> >>
>> >>
>> >> As this is an early PoC, there are still a few missing pieces before
>> >> this can be merged. Including (but probably not limited to):
>> >>
>> >> - Consolidate the map definition struct in the bpf_helpers.h file in the
>> >>   kernel tree. This contains a different, and incompatible, update to
>> >>   the struct. Since the iproute2 version has actually been released for
>> >>   use outside the kernel tree (and thus is subject to API stability
>> >>   constraints), I think it makes the most sense to keep that, and port
>> >>   the selftests to use it.
>> >
>> > It sounds like you're implying that existing libbpf format is not
>> > uapi.
>>
>> No, that's not what I meant... See below.
>>
>> > It is and we cannot break it.
>> > If patch 1 means breakage for existing pre-compiled .o that won't load
>> > with new libbpf then we cannot use this method.
>> > Recompiling .o with new libbpf definition of bpf_map_def isn't an option.
>> > libbpf has to be smart before/after and recognize both old and iproute2 format.
>>
>> The libbpf.h definition of struct bpf_map_def is compatible with the one
>> used in iproute2. In libbpf.h, the struct only contains five fields
>> (type, key_size, value_size, max_entries and flags), and iproute2 adds
>> another 4 (id, pinning, inner_id and inner_idx; these are the ones in
>> patch 1 in this series).
>>
>> The issue I was alluding to above is that the bpf_helpers.h file in the
>> kernel selftests directory *also* extends the bpf_map_def struct, and
>> adds two *different* fields (inner_map_idx and numa_mode). The former is
>> used to implement the same map-in-map definition functionality that
>> iproute2 has, but with different semantics. The latter is additional to
>> that, and I'm planning to add that to this series.
>>
>> Since bpf_helpers.h is *not* part of libbpf (yet), this will make it
>
> We should start considering it as if it was, so if we can avoid adding
> stuff that I'd need to untangle to move it into libbpf, I'd rather
> avoid it.
> We've already prepared this move by relicensing bpf_helpers.h. Moving
> it into libbpf itself is immediate next thing I'll do when I'm back.

Yeah, I figured that with the relicensing, bpf_helpers would probably be
making its way into libbpf soon. Which is why I wanted to start this
discussion before that: If we do move bpf_helpers as-is, that will put
us in the territory of full-on binary incompatibility. So the time to
discuss doing this in a compatible way is now, before any such move is
made.

-Toke