Message-ID: <CACdoK4+4iB-aquCgxXV9BYcXZpPrREZrnm+G8hau-SOt8QPtqQ@mail.gmail.com>
Date:   Thu, 7 Oct 2021 20:43:11 +0100
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 v3 00/10] install libbpf headers when using the library

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:

... 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?

Thanks,
Quentin

Powered by blists - more mailing lists