Message-ID: <87r1jyth94.fsf@toke.dk>
Date:   Mon, 29 Mar 2021 11:56:55 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Kumar Kartikeya Dwivedi <memxor@...il.com>,
        bpf <bpf@...r.kernel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        open list <linux-kernel@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf-next 5/5] libbpf: add selftests for TC-BPF API

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> On Sat, Mar 27, 2021 at 09:32:58PM -0700, Andrii Nakryiko wrote:
>> > I think it's better to start with new library for tc/xdp and have
>> > libbpf as a dependency on that new lib.
>> > For example we can add it as subdir in tools/lib/bpf/.
>> >
>> > Similarly I think integerating static linking into libbpf was a mistake.
>> > It should be a sub library as well.
>> >
>> > If we end up with core libbpf and ten sublibs for tc, xdp, af_xdp, linking,
>> > whatever else the users would appreciate that we don't shove single libbpf
>> > to them with a ton of features that they might never use.
>> 
>> What's the concern exactly? The size of the library? Having 10
>> micro-libraries has its own set of downsides, 
>
> specifically?
>
>> I'm not convinced that's
>> a better situation for end users. And would certainly cause more
>> hassle for libbpf developers and packagers.
>
> For developers and packagers.. yes.
> For users.. quite the opposite.
> The skel gen and static linking must be split out before the next libbpf release.
> Not a single application linked with libbpf is going to use those pieces.

I'd tend to agree about the skeleton generation, but I have one use case
in mind where having the linker in library form would be handy:
dynamically building an XDP program at load time from pre-compiled
pieces.

Consider xdp-filter[0]: it's a simplistic packet filter that can filter
on different bits of the packet header, mostly meant as a demonstration
of XDP packet filtering performance. It's also using conditional
compilation so that it can be loaded in a mode that skips parsing L4
headers entirely if port-based filtering is not enabled. Right now we do
that by pre-compiling five different variants of the XDP program and
loading based on the selected feature set, but with linking in libbpf,
we could instead have a single BPF program with granular filtering
functions and just assemble the final program from those bits at load
time.

The actual xdp-filter program may be too simplistic to gain any
performance for this, but I believe the general approach could be a way
to realise the "improved performance through skipping code" promise of
an XDP-based data path. Having linking be part of libbpf will make this
straight-forward to integrate into applications.

[0] https://github.com/xdp-project/xdp-tools/tree/master/xdp-filter

> bpftool is one and only that needs them. Hence forcing libbpf users
> to increase their .text with a dead code is a selfish call of libbpf
> developers and packagers. The user's priorities must come first.
>
>> And what did you include in "core libbpf"?
>
> I would take this opportunity to split libbpf into maintainable pieces:
> - libsysbpf - sys_bpf wrappers (pretty much tools/lib/bpf/bpf.c)
> - libbpfutil - hash, strset
> - libbtf - BTF read/write
> - libbpfelf - ELF parsing, CORE, ksym, kconfig
> - libbpfskel - skeleton gen used by bpftool only
> - libbpflink - linker used by bpftool only
> - libbpfnet - networking attachment via netlink including TC and XDP
> - libbpftrace - perfbuf, ringbuf
> - libxdp - Toke's xdp chaining
> - libxsk - af_xdp logic

Huh? You've got to be joking? How is that going to improve things for
users? Just the cognitive load of figuring out which linker flags to use
is going to be prohibitive. Not to mention the hassle of keeping
multiple library versions in sync etc.

If the concern is .text size, surely there are better ways to fix that?
LTO is the obvious "automagic" solution, but even without that, just
supporting conditional compilation via defines in the existing libbpf
ought to achieve the same thing without exposing the gory details to the
users?

-Toke

Powered by blists - more mailing lists