| Message ID | 20211003192208.6297-1-quentin@isovalent.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | install libbpf headers when using the library | expand |
On Sun, Oct 3, 2021 at 12:22 PM Quentin Monnet <quentin@isovalent.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. 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! > - Remove order-only dependencies on $(LIBBPF_INCLUDE) (or equivalent) > directories, given that they are created by libbpf's Makefile. > - Add libbpf as a dependency for bpftool/resolve_btfids/runqslower when > they are supposed to reuse a libbpf compiled previously. This is to > avoid having several libbpf versions being compiled simultaneously in > the same directory with parallel builds. Even if this didn't show up > during tests, let's remain on the safe side. > - kernel/bpf/preload/Makefile: Rename libbpf-hdrs (dash) dependency as > libbpf_hdrs. > - samples/bpf/.gitignore: Add bpftool/ > - samples/bpf/Makefile: Change "/bin/rm -rf" to "$(RM) -r". > - samples/bpf/Makefile: Add missing slashes for $(LIBBPF_OUTPUT) and > $(LIBBPF_DESTDIR) when buildling bpftool > - samples/bpf/Makefile: Add a dependency to libbpf's headers for > $(TRACE_HELPERS). > - bpftool's Makefile: Use $(LIBBPF) instead of equivalent (but longer) > $(LIBBPF_OUTPUT)libbpf.a > - BPF iterators' Makefile: build bpftool in .output/bpftool (instead of > .output/), add and clean up variables. > - runqslower's Makefile: Add an explicit dependency on libbpf's headers > to several objects. The dependency is not required (libbpf should have > been compiled and so the headers exported through other dependencies > for those targets), but they better mark the logical dependency and > should help if exporting the headers changed in the future. > - New commit to add an "install-bin" target to bpftool, to avoid > installing bash completion when buildling BPF iterators and selftests. > > 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 (10): > 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 > tools: bpftool: add install-bin target to install binary only > > kernel/bpf/preload/Makefile | 25 ++++++++--- > kernel/bpf/preload/iterators/Makefile | 39 ++++++++++++------ > samples/bpf/.gitignore | 4 ++ > samples/bpf/Makefile | 41 ++++++++++++++----- > tools/bpf/bpftool/Makefile | 32 +++++++++------ > 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 | 22 ++++++---- > tools/testing/selftests/bpf/Makefile | 26 ++++++++---- > .../selftests/bpf/test_bpftool_build.sh | 4 ++ > 12 files changed, 148 insertions(+), 68 deletions(-) > > -- > 2.30.2 >
On Wed, 6 Oct 2021 at 19:28, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sun, Oct 3, 2021 at 12:22 PM Quentin Monnet <quentin@isovalent.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
On Thu, Oct 7, 2021 at 12:43 PM Quentin Monnet <quentin@isovalent.com> wrote: > > On Wed, 6 Oct 2021 at 19:28, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Sun, Oct 3, 2021 at 12:22 PM Quentin Monnet <quentin@isovalent.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
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: - Remove order-only dependencies on $(LIBBPF_INCLUDE) (or equivalent) directories, given that they are created by libbpf's Makefile. - Add libbpf as a dependency for bpftool/resolve_btfids/runqslower when they are supposed to reuse a libbpf compiled previously. This is to avoid having several libbpf versions being compiled simultaneously in the same directory with parallel builds. Even if this didn't show up during tests, let's remain on the safe side. - kernel/bpf/preload/Makefile: Rename libbpf-hdrs (dash) dependency as libbpf_hdrs. - samples/bpf/.gitignore: Add bpftool/ - samples/bpf/Makefile: Change "/bin/rm -rf" to "$(RM) -r". - samples/bpf/Makefile: Add missing slashes for $(LIBBPF_OUTPUT) and $(LIBBPF_DESTDIR) when buildling bpftool - samples/bpf/Makefile: Add a dependency to libbpf's headers for $(TRACE_HELPERS). - bpftool's Makefile: Use $(LIBBPF) instead of equivalent (but longer) $(LIBBPF_OUTPUT)libbpf.a - BPF iterators' Makefile: build bpftool in .output/bpftool (instead of .output/), add and clean up variables. - runqslower's Makefile: Add an explicit dependency on libbpf's headers to several objects. The dependency is not required (libbpf should have been compiled and so the headers exported through other dependencies for those targets), but they better mark the logical dependency and should help if exporting the headers changed in the future. - New commit to add an "install-bin" target to bpftool, to avoid installing bash completion when buildling BPF iterators and selftests. 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 (10): 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 tools: bpftool: add install-bin target to install binary only kernel/bpf/preload/Makefile | 25 ++++++++--- kernel/bpf/preload/iterators/Makefile | 39 ++++++++++++------ samples/bpf/.gitignore | 4 ++ samples/bpf/Makefile | 41 ++++++++++++++----- tools/bpf/bpftool/Makefile | 32 +++++++++------ 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 | 22 ++++++---- tools/testing/selftests/bpf/Makefile | 26 ++++++++---- .../selftests/bpf/test_bpftool_build.sh | 4 ++ 12 files changed, 148 insertions(+), 68 deletions(-)