Date:   Thu, 7 Oct 2021 14:24:14 -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 v3 00/10] install libbpf headers when using the library

On Thu, Oct 7, 2021 at 12:43 PM Quentin Monnet <quentin@...valent.com> wrote:
>
> On Wed, 6 Oct 2021 at 19:28, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >
> > On Sun, Oct 3, 2021 at 12:22 PM Quentin Monnet <quentin@...valent.com> wrote:
> > >
> > > Libbpf is used at several locations in the repository. Most of the time,
> > > the tools relying on it build the library in its own directory, and include
> > > the headers from there. This works, but this is not the cleanest approach.
> > > It generates objects outside of the directory of the tool which is being
> > > built, and it also increases the risk that developers include a header file
> > > internal to libbpf, which is not supposed to be exposed to user
> > > applications.
> > >
> > > This set adjusts all involved Makefiles to make sure that libbpf is built
> > > locally (with respect to the tool's directory or provided build directory),
> > > and by ensuring that "make install_headers" is run from libbpf's Makefile
> > > to export user headers properly.
> > >
> > > This comes at a cost: given that the libbpf was so far mostly compiled in
> > > its own directory by the different components using it, compiling it once
> > > would be enough for all those components. With the new approach, each
> > > component compiles its own version. To mitigate this cost, efforts were
> > > made to reuse the compiled library when possible:
> > >
> > > - Make the bpftool version in samples/bpf reuse the library previously
> > >   compiled for the selftests.
> > > - Make the bpftool version in BPF selftests reuse the library previously
> > >   compiled for the selftests.
> > > - Similarly, make resolve_btfids in BPF selftests reuse the same compiled
> > >   library.
> > > - Similarly, make runqslower in BPF selftests reuse the same compiled
> > >   library; and make it rely on the bpftool version also compiled from the
> > >   selftests (instead of compiling its own version).
> > > - runqslower, when compiled independently, needs its own version of
> > >   bpftool: make them share the same compiled libbpf.
> > >
> > > As a result:
> > >
> > > - Compiling the samples/bpf should compile libbpf just once.
> > > - Compiling the BPF selftests should compile libbpf just once.
> > > - Compiling the kernel (with BTF support) should now lead to compiling
> > >   libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
> > > - Compiling runqslower individually should compile libbpf just once. Same
> > >   thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.
> > >
> > > (Not accounting for the boostrap version of libbpf required by bpftool,
> > > which was already placed under a dedicated .../boostrap/libbpf/ directory,
> > > and for which the count remains unchanged.)
> > >
> > > A few commits in the series also contain drive-by clean-up changes for
> > > bpftool includes, samples/bpf/.gitignore, or test_bpftool_build.sh. Please
> > > refer to individual commit logs for details.
> > >
> > > v3:
> >
> > Please see few problems with libbpf_hdrs phony targets. Seems like
> > they all can be order-only dependencies and not causing unnecessary
> > rebuilds.
>
> Nice catch, I didn't realise it would force rebuilding :(. I'll
> address it in the next version. I'll also add a few adjustments to
> libbpf's and bpftool's Makefiles to make sure we don't recompile when
> not necessary, because of the header files that are currently
> installed unconditionally.
>
> > Can you please also normalize your patch prefixes for bpftool and
> > other tools? We've been using a short and simple "bpftool: " prefix
> > for bpftool-related changes, and for other tools it would be just
> > "tools/runqslower" or "tools/resolve_btfids". Please update
> > accordingly. Thanks!
>
> $ git log --oneline --pretty='format:%s' -- tools/bpf/bpftool/ | \
>         grep -oE '^(bpftool:|tools: bpftool:)' | sort | uniq -c
>    128 bpftool:
>    194 tools: bpftool:
>

But then:

$ git log --oneline --pretty='format:%s' -- tools/testing/selftests/bpf/ | \
        grep -oE '^(selftests/bpf:|selftests: bpf:)' | sort | uniq -c
    925 selftests/bpf:
     98 selftests: bpf:

And if we expand your search a bit:

$ git log --oneline --pretty='format:%s' -- tools/bpf/bpftool/ | \
         grep -oE '^(bpftool:|tools: bpftool:|tools/bpftool:)' | sort | uniq -c
    130 bpftool:
     52 tools/bpftool:
    194 tools: bpftool:

bpftool: + tools/bpftool: almost matches up with tools: bpftool: ;)

I think the most prevailing convention was "dir1/dir2: " style overall.

> ... And “we”'ve been using “tools: bpftool:” since the early days :).
> But yeah sure, I'll adjust. Shorter looks better. Just wondering, are
> those prefixes documented anywhere?

I don't think so.


>
> Thanks,
> Quentin

Powered by blists - more mailing lists