| Message ID | 20220424051022.2619648-2-asmadeus@codewreck.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | BPF |
| Headers | show |
| Series | tools/bpf: allow building with musl | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest + selftests |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on z15 + selftests |
| netdev/tree_selection | success | Not a local patch |
Dominique Martinet wrote on Sun, Apr 24, 2022 at 02:10:19PM +0900: > After having done this work I noticed runqslower is not actually > installed, so ideally instead of all of this it'd make more sense to > just not build it: would it make sense to take it out of the defaults > build targets? > I could just directly build the appropriate targets from tools/bpf > directory with 'make bpftool bpf_dbg bpf_asm bpf_jit_disasm', but > ideally I'd like to keep alpine's build script way of calling make from > the tools parent directory, and 'make bpf' there is all or nothing. Well, it turns out runqslower doesn't build if the current kernel or vmlinux in tree don't have BTF enabled, so the current alpine builder can't build it. I've dropped this patch from my alpine MR[1] and built things directly with make bpftool etc as suggested above, so my suggestion to make it more easily buildable that way is probably the way to go? [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/33554 Thanks,
On 4/24/22 8:58 AM, Dominique Martinet wrote: > Dominique Martinet wrote on Sun, Apr 24, 2022 at 02:10:19PM +0900: >> After having done this work I noticed runqslower is not actually >> installed, so ideally instead of all of this it'd make more sense to >> just not build it: would it make sense to take it out of the defaults >> build targets? >> I could just directly build the appropriate targets from tools/bpf >> directory with 'make bpftool bpf_dbg bpf_asm bpf_jit_disasm', but >> ideally I'd like to keep alpine's build script way of calling make from >> the tools parent directory, and 'make bpf' there is all or nothing. > > Well, it turns out runqslower doesn't build if the current kernel or > vmlinux in tree don't have BTF enabled, so the current alpine builder > can't build it. > > I've dropped this patch from my alpine MR[1] and built things directly > with make bpftool etc as suggested above, so my suggestion to make it > more easily buildable that way is probably the way to go? > [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/33554 Thanks for looking into this, Dominique! I slightly massaged patch 3 & 4 and applied it to bpf-next tree. I don't really mind about patch 1 & 2, though out of tools/bpf/ the only one you /really/ might want to package is bpftool. The other tools are on the legacy side of things and JIT disasm you can also get via bpftool anyway. Given this is not covered by BPF CI, are you planning to regularly check for musl compatibility before a new kernel is cut? Thanks, Daniel
Daniel Borkmann wrote on Mon, Apr 25, 2022 at 11:35:41PM +0200: > > I've dropped this patch from my alpine MR[1] and built things directly > > with make bpftool etc as suggested above, so my suggestion to make it > > more easily buildable that way is probably the way to go? > > [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/33554 > > Thanks for looking into this, Dominique! I slightly massaged patch 3 & 4 > and applied it to bpf-next tree. Thanks! > I don't really mind about patch 1 & 2, though out of tools/bpf/ the only > one you /really/ might want to package is bpftool. The other tools are on > the legacy side of things and JIT disasm you can also get via bpftool anyway. I was thinking the other tools still had their uses, but I'll readily admit I've never had a need for them so wasn't sure if I should package them together or not. I can see the use of bpf_dbg, but it's occasional enough that people who need it can just build it when they need... Let's drop both patches and I'll remove the other legacy tools from package as well. My last concern would then just be to build it more easily. I just noticed I can actually 'make bpf/bpftool' directly from the tools/ parent directory, but there's no equivalent for _install rules. Would something like this make sense? (I can resend as proper patch if so) ---- diff --git a/tools/Makefile b/tools/Makefile index db2f7b8ebed5..743d242aebb3 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -112,6 +112,9 @@ cpupower_install: cgroup_install counter_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install: $(call descend,$(@:_install=),install) +bpf/%_install: FORCE + $(call descend,$(@:_install=),install) + selftests_install: $(call descend,testing/$(@:_install=),install) ---- > Given this is not covered by BPF CI, are you planning to regularly check > for musl compatibility before a new kernel is cut? alpine doesn't update the 'tools' subpackage with every kernel release, I'm not sure what the exact schedule is but from the looks of it it tracks LTS releases with updates every few months within the stable release or to the next one. I don't really have any resource to run a regular CI, but I guess I can check from time to time.. If I ever get around to adding a linux-next test to work's CI I can check bpftool builds at the same time, but who knows when that'll ever be. OTOH I had a first look last year (back when I tried to push ACTIONRETVAL to musl) and there haven't been any new incompatibility, so I think it's fine to just deal with minor hiccups when alpine upgrades once in a while.
diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile index da6de16a3dfb..20a1d9a2a908 100644 --- a/tools/bpf/runqslower/Makefile +++ b/tools/bpf/runqslower/Makefile @@ -23,6 +23,34 @@ VMLINUX_BTF_PATHS := $(if $(O),$(O)/vmlinux) \ VMLINUX_BTF_PATH := $(or $(VMLINUX_BTF),$(firstword \ $(wildcard $(VMLINUX_BTF_PATHS)))) +# musl requires linking with an external libargp +FEATURE_USER = .runqslower +FEATURE_TEST = libargp +FEATURE_DISPLAY = + +check_feat := 1 +NON_CHECK_FEAT_TARGETS := clean +ifdef MAKECMDGOALS +ifeq ($(filter-out $(NON_CHECK_FEAT_TARGETS),$(MAKECMDGOALS)),) + check_feat := 0 +endif +endif + +ifeq ($(check_feat),1) +ifeq ($(FEATURES_DUMP),) +srctree := $(abspath ../../..) +include $(srctree)/tools/build/Makefile.feature +else +include $(FEATURES_DUMP) +endif +endif + +LIBS = -lelf -lz +$(call feature_check,libargp) +ifeq ($(feature-libargp), 1) +LIBS += -largp +endif + ifeq ($(V),1) Q = else @@ -49,7 +77,7 @@ clean: libbpf_hdrs: $(BPFOBJ) $(OUTPUT)/runqslower: $(OUTPUT)/runqslower.o $(BPFOBJ) - $(QUIET_LINK)$(CC) $(CFLAGS) $^ -lelf -lz -o $@ + $(QUIET_LINK)$(CC) $(CFLAGS) $^ $(LIBS) -o $@ $(OUTPUT)/runqslower.o: runqslower.h $(OUTPUT)/runqslower.skel.h \ $(OUTPUT)/runqslower.bpf.o | libbpf_hdrs diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile index de66e1cc0734..ceb4224a0ede 100644 --- a/tools/build/feature/Makefile +++ b/tools/build/feature/Makefile @@ -37,6 +37,7 @@ FILES= \ test-libtraceevent.bin \ test-libtracefs.bin \ test-libcrypto.bin \ + test-libargp.bin \ test-libunwind.bin \ test-libunwind-debug-frame.bin \ test-libunwind-x86.bin \ @@ -205,6 +206,9 @@ $(OUTPUT)test-libtracefs.bin: $(OUTPUT)test-libcrypto.bin: $(BUILD) -lcrypto +$(OUTPUT)test-libargp.bin: + $(BUILD) -largp + $(OUTPUT)test-gtk2.bin: $(BUILD) $(shell $(PKG_CONFIG) --libs --cflags gtk+-2.0 2>/dev/null) -Wno-deprecated-declarations diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c index 5ffafb967b6e..149d3ef4a439 100644 --- a/tools/build/feature/test-all.c +++ b/tools/build/feature/test-all.c @@ -146,6 +146,10 @@ # include "test-libcrypto.c" #undef main +#define main main_test_libargp +# include "test-libargp.c" +#undef main + #define main main_test_sdt # include "test-sdt.c" #undef main diff --git a/tools/build/feature/test-libargp.c b/tools/build/feature/test-libargp.c new file mode 100644 index 000000000000..63b65d1f11fe --- /dev/null +++ b/tools/build/feature/test-libargp.c @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <argp.h> + +const char *argp_program_version = "test-libargp"; +static const struct argp_option opts[] = { {} }; + +int main(int argc, char **argv) +{ + static const struct argp argp = { + .options = opts, + }; + argp_parse(&argp, argc, argv, 0, NULL, NULL); + return 0; +}
musl doesn't implement argp.h and requires an explicit lib for it, so we must test for -largp presence and use it if appropriate Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- After having done this work I noticed runqslower is not actually installed, so ideally instead of all of this it'd make more sense to just not build it: would it make sense to take it out of the defaults build targets? I could just directly build the appropriate targets from tools/bpf directory with 'make bpftool bpf_dbg bpf_asm bpf_jit_disasm', but ideally I'd like to keep alpine's build script way of calling make from the tools parent directory, and 'make bpf' there is all or nothing. OTOH, we might as well keep this to allow people on alpine/void linux to build runqslower if they want to. I didn't add libargp to default features check so it shouldn't change much except for runqslower itself. As an example it might be better to keep it independant from kbuild but it already wasn't so I don't see much harm here. tools/bpf/runqslower/Makefile | 30 +++++++++++++++++++++++++++++- tools/build/feature/Makefile | 4 ++++ tools/build/feature/test-all.c | 4 ++++ tools/build/feature/test-libargp.c | 14 ++++++++++++++ 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 tools/build/feature/test-libargp.c