Viktor Malik <vmalik@xxxxxxxxxx> writes: > On 11/7/24 11:33, Toke Høiland-Jørgensen wrote: >> The selftests build four kernel modules which use copy-pasted Makefile >> targets. This is a bit messy, and doesn't scale so well when we add more >> modules, so let's consolidate these rules into a single rule generated >> for each module name, and move the module sources into a single >> directory. >> >> To avoid parallel builds of the different modules stepping on each >> other's toes during the 'modpost' phase of the Kbuild 'make modules', we >> create a single target for all the defined modules, which contains the >> recursive 'make' call into the modules directory. The Makefile in the >> subdirectory building the modules is modified to have a .PHONY target >> which also touches a 'modules.built' file. This way we can add this file >> as a dependency on the top-level selftests Makefile, thus ensuring that >> the modules are always rebuilt if any of the dependencies in the >> selftests change. The .PHONY target doesn't cause spurious rebuilds >> since we track all the dependencies in the parent directory Makefile and >> only call make in the subdirectory if anything changes. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx> >> --- >> Changes in v2: >> - Rebase on bpf-next, incorporating Viktor's EXTRA_CFLAGS patch >> - A few small adjustments to the module Makefile recipe >> - Link to v1: https://lore.kernel.org/r/20241031-bpf-selftests-mod-compile-v1-1-1a63af2385f1@xxxxxxxxxx >> --- >> tools/testing/selftests/bpf/Makefile | 62 +++++++--------------- >> .../selftests/bpf/bpf_test_modorder_x/Makefile | 19 ------- >> .../selftests/bpf/bpf_test_modorder_y/Makefile | 19 ------- >> .../testing/selftests/bpf/bpf_test_no_cfi/Makefile | 19 ------- >> tools/testing/selftests/bpf/bpf_testmod/Makefile | 20 ------- >> .../testing/selftests/bpf/prog_tests/core_reloc.c | 2 +- >> tools/testing/selftests/bpf/progs/bad_struct_ops.c | 2 +- >> tools/testing/selftests/bpf/progs/cb_refs.c | 2 +- >> tools/testing/selftests/bpf/progs/epilogue_exit.c | 4 +- >> .../selftests/bpf/progs/epilogue_tailcall.c | 4 +- >> tools/testing/selftests/bpf/progs/iters_testmod.c | 2 +- >> tools/testing/selftests/bpf/progs/jit_probe_mem.c | 2 +- >> .../selftests/bpf/progs/kfunc_call_destructive.c | 2 +- >> .../testing/selftests/bpf/progs/kfunc_call_fail.c | 2 +- >> .../testing/selftests/bpf/progs/kfunc_call_race.c | 2 +- >> .../testing/selftests/bpf/progs/kfunc_call_test.c | 2 +- >> .../selftests/bpf/progs/kfunc_call_test_subprog.c | 2 +- >> .../testing/selftests/bpf/progs/local_kptr_stash.c | 2 +- >> tools/testing/selftests/bpf/progs/map_kptr.c | 2 +- >> tools/testing/selftests/bpf/progs/map_kptr_fail.c | 2 +- >> tools/testing/selftests/bpf/progs/missed_kprobe.c | 2 +- >> .../selftests/bpf/progs/missed_kprobe_recursion.c | 2 +- >> tools/testing/selftests/bpf/progs/nested_acquire.c | 2 +- >> tools/testing/selftests/bpf/progs/pro_epilogue.c | 4 +- >> .../selftests/bpf/progs/pro_epilogue_goto_start.c | 4 +- >> tools/testing/selftests/bpf/progs/sock_addr_kern.c | 2 +- >> .../selftests/bpf/progs/struct_ops_detach.c | 2 +- >> .../selftests/bpf/progs/struct_ops_forgotten_cb.c | 2 +- >> .../selftests/bpf/progs/struct_ops_maybe_null.c | 2 +- >> .../bpf/progs/struct_ops_maybe_null_fail.c | 2 +- >> .../selftests/bpf/progs/struct_ops_module.c | 2 +- >> .../selftests/bpf/progs/struct_ops_multi_pages.c | 2 +- >> .../selftests/bpf/progs/struct_ops_nulled_out_cb.c | 2 +- >> .../bpf/progs/test_kfunc_param_nullable.c | 2 +- >> .../selftests/bpf/progs/test_module_attach.c | 2 +- >> .../selftests/bpf/progs/test_tp_btf_nullable.c | 2 +- >> .../testing/selftests/bpf/progs/unsupported_ops.c | 2 +- >> tools/testing/selftests/bpf/progs/wq.c | 2 +- >> tools/testing/selftests/bpf/progs/wq_failures.c | 2 +- >> .../bpf/{bpf_testmod => test_kmods}/.gitignore | 0 >> tools/testing/selftests/bpf/test_kmods/Makefile | 25 +++++++++ >> .../bpf_test_modorder_x.c | 0 >> .../bpf_test_modorder_y.c | 0 >> .../bpf_test_no_cfi.c | 0 >> .../bpf_testmod-events.h | 0 >> .../bpf/{bpf_testmod => test_kmods}/bpf_testmod.c | 0 >> .../bpf/{bpf_testmod => test_kmods}/bpf_testmod.h | 0 >> .../bpf_testmod_kfunc.h | 0 >> 48 files changed, 82 insertions(+), 158 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile >> index edef5df08cb2536260f8910b2ebd2b89dbd0ebd2..1c35e29e3e94d86eb5619db5cb20e2d42772fe60 100644 >> --- a/tools/testing/selftests/bpf/Makefile >> +++ b/tools/testing/selftests/bpf/Makefile >> @@ -152,13 +152,15 @@ TEST_PROGS_EXTENDED := with_addr.sh \ >> with_tunnels.sh ima_setup.sh verify_sig_setup.sh \ >> test_xdp_vlan.sh test_bpftool.py >> >> +TEST_KMODS := bpf_testmod.ko bpf_test_no_cfi.ko bpf_test_modorder_x.ko \ >> + bpf_test_modorder_y.ko >> +TEST_KMOD_TARGETS = $(addprefix $(OUTPUT)/,$(TEST_KMODS)) >> + >> # Compile but not part of 'make run_tests' >> TEST_GEN_PROGS_EXTENDED = \ >> flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \ >> - test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \ >> - xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \ >> - xdp_features bpf_test_no_cfi.ko bpf_test_modorder_x.ko \ >> - bpf_test_modorder_y.ko >> + test_lirc_mode2_user xdping test_cpp runqslower bench xskxceiver \ >> + xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata xdp_features >> >> TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi >> >> @@ -173,8 +175,9 @@ override define CLEAN >> $(Q)$(RM) -r $(TEST_GEN_PROGS) >> $(Q)$(RM) -r $(TEST_GEN_PROGS_EXTENDED) >> $(Q)$(RM) -r $(TEST_GEN_FILES) >> + $(Q)$(RM) -r $(TEST_KMODS) >> $(Q)$(RM) -r $(EXTRA_CLEAN) >> - $(Q)$(MAKE) -C bpf_testmod clean >> + $(Q)$(MAKE) -C test_kmods clean >> $(Q)$(MAKE) docs-clean >> endef >> >> @@ -240,7 +243,7 @@ endif >> # to build individual tests. >> # NOTE: Semicolon at the end is critical to override lib.mk's default static >> # rule for binaries. >> -$(notdir $(TEST_GEN_PROGS) \ >> +$(notdir $(TEST_GEN_PROGS) $(TEST_KMODS) \ >> $(TEST_GEN_PROGS_EXTENDED)): %: $(OUTPUT)/% ; >> >> # sort removes libbpf duplicates when not cross-building >> @@ -294,37 +297,15 @@ $(OUTPUT)/sign-file: ../../../../scripts/sign-file.c >> $< -o $@ \ >> $(shell $(PKG_CONFIG) --libs libcrypto 2> /dev/null || echo -lcrypto) >> >> -$(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(RESOLVE_BTFIDS) $(wildcard bpf_testmod/Makefile bpf_testmod/*.[ch]) >> - $(call msg,MOD,,$@) >> - $(Q)$(RM) bpf_testmod/bpf_testmod.ko # force re-compilation >> - $(Q)$(MAKE) $(submake_extras) -C bpf_testmod \ >> - RESOLVE_BTFIDS=$(RESOLVE_BTFIDS) \ >> +test_kmods/modules.built: $(VMLINUX_BTF) $(RESOLVE_BTFIDS) $(wildcard test_kmods/Makefile test_kmods/*.[ch]) > > One problem with this approach is that modules are not recompiled after > the user manually removes the .ko files: > > $ find -name "*.ko" -delete > $ make > MOD bpf_testmod.ko > cp: cannot stat 'test_kmods/bpf_testmod.ko': No such file or directory > make: *** [Makefile:308: > /bpf-next/tools/testing/selftests/bpf/bpf_testmod.ko] Error 1 > > Not sure if that's a common use-case but it feels like one way to force > recompilation of modules so people may actually want to use it. Yeah, fair point. I played around with it a bit more, and I think I found a way (using .NOTPARALLEL) to make this work (and get rid of the 'modules.built' file entirely). Will respin. >> + $(Q)$(RM) test_kmods/*.ko test_kmods/*.mod.o # force re-compilation > > This means that we always recompile all modules, right? IMHO it's not a > problem (at least not at the moment as there are few modules and the > compilation is fast) but I'm just pointing it out. Yeah, it does. I don't think this is a huge issue, though, and with the .NOTPARALLEL approach it actually helps to rebuild them all in one go, as we'll otherwise only rebuild one module (if there's a change in one of the prereqs), but we'll still end up with no-op recursive make calls for the other modules because make can't properly track dependencies across the kernel module build (you can try removing the 'rm' from v3 to see what I mean). So all in all I think this is OK. -Toke
