On Fri, Oct 1, 2021 at 4:05 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> 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 > >
