From: Andrii Nakryiko <email@example.com> To: Quentin Monnet <firstname.lastname@example.org> Cc: Alexei Starovoitov <email@example.com>, Daniel Borkmann <firstname.lastname@example.org>, Andrii Nakryiko <email@example.com>, Networking <firstname.lastname@example.org>, bpf <email@example.com> Subject: Re: [PATCH bpf-next v2 0/9] install libbpf headers when using the library Date: Fri, 1 Oct 2021 16:27:40 -0700 [thread overview] Message-ID: <CAEf4Bzavcr56jmfA7GSqbN78o93rcpAMqi3mGj3pA-xzL6yWnA@mail.gmail.com> (raw) In-Reply-To: <CAEf4Bza+C5cNJbvw_n_pR_mVL0rPH2VkZd-AJMx78Fp_m+CpRQ@mail.gmail.com> On Fri, Oct 1, 2021 at 4:05 PM Andrii Nakryiko <firstname.lastname@example.org> wrote: > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <email@example.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. > > The whole sharing of libbpf build artifacts is great, I just want to > point out that it's also dangerous if those multiple Makefiles aren't > ordered properly. E.g., if you build runqslower and the rest of > selftests in parallel without making sure that libbpf already > completed its build, you might end up building libbpf in parallel in > two independent make instances and subsequently corrupting generated > object files. I haven't looked through all the changes (and I'll > confess that it's super hard to reason about dependencies and ordering > in Makefile) and I'll keep this in mind, but wanted to bring this up. > I suspect you already thought about that, but would be worth to call > out this explicitly. > Ok, I looked through the patches. It all looks reasonable to me, but as I mentioned, Makefile is pure magic and human brain can't keep track of all those dependencies and their ordering. I tried these changes locally and so far all the builds I usually do work fine. But it would be great if a few more folks try these changes locally for their normal workflows to help test this. If not, we can land this early next week, so that we have a whole week in front of us to fix whatever fallout there will be due to these changes. Sounds good? > > > > (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. > > > > v2: Declare an additional dependency on libbpf's headers for > > iterators/iterators.o in kernel/preload/Makefile to make sure that > > these headers are exported before we compile the object file (and not > > just before we link it). > > > > Quentin Monnet (9): > > tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h> > > tools: bpftool: install libbpf headers instead of including the dir > > tools: resolve_btfids: install libbpf headers when building > > tools: runqslower: install libbpf headers when building > > bpf: preload: install libbpf headers when building > > bpf: iterators: install libbpf headers when building > > samples/bpf: install libbpf headers when building > > samples/bpf: update .gitignore > > selftests/bpf: better clean up for runqslower in test_bpftool_build.sh > > > > kernel/bpf/preload/Makefile | 25 ++++++++++--- > > kernel/bpf/preload/iterators/Makefile | 18 ++++++---- > > samples/bpf/.gitignore | 3 ++ > > samples/bpf/Makefile | 36 +++++++++++++------ > > tools/bpf/bpftool/Makefile | 27 ++++++++------ > > tools/bpf/bpftool/gen.c | 1 - > > tools/bpf/bpftool/prog.c | 1 - > > tools/bpf/resolve_btfids/Makefile | 17 ++++++--- > > tools/bpf/resolve_btfids/main.c | 4 +-- > > tools/bpf/runqslower/Makefile | 12 ++++--- > > tools/testing/selftests/bpf/Makefile | 22 ++++++++---- > > .../selftests/bpf/test_bpftool_build.sh | 4 +++ > > 12 files changed, 116 insertions(+), 54 deletions(-) > > > > -- > > 2.30.2 > >
next prev parent reply other threads:[~2021-10-01 23:27 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-01 11:08 Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 1/9] tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h> Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir Quentin Monnet 2021-10-01 22:55 ` Andrii Nakryiko 2021-10-01 11:08 ` [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building Quentin Monnet 2021-10-01 23:01 ` Andrii Nakryiko 2021-10-01 11:08 ` [PATCH bpf-next v2 4/9] tools: runqslower: " Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 5/9] bpf: preload: " Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 6/9] bpf: iterators: " Quentin Monnet 2021-10-01 23:19 ` Andrii Nakryiko 2021-10-02 20:27 ` Quentin Monnet 2021-10-02 22:11 ` Quentin Monnet 2021-10-04 19:10 ` Andrii Nakryiko 2021-10-04 21:30 ` Quentin Monnet 2021-10-05 20:03 ` Quentin Monnet 2021-10-06 17:44 ` Andrii Nakryiko 2021-10-01 11:08 ` [PATCH bpf-next v2 7/9] samples/bpf: " Quentin Monnet 2021-10-01 23:22 ` Andrii Nakryiko 2021-10-02 20:30 ` Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 8/9] samples/bpf: update .gitignore Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 9/9] selftests/bpf: better clean up for runqslower in test_bpftool_build.sh Quentin Monnet 2021-10-01 23:05 ` [PATCH bpf-next v2 0/9] install libbpf headers when using the library Andrii Nakryiko 2021-10-01 23:27 ` Andrii Nakryiko [this message] 2021-10-02 20:40 ` Quentin Monnet 2021-10-03 19:20 ` Quentin Monnet
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAEf4Bzavcr56jmfA7GSqbN78o93rcpAMqi3mGj3pA-xzL6yWnA@mail.gmail.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH bpf-next v2 0/9] install libbpf headers when using the library' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).