Re: [PATCH bpf-next v2] selftests/bpf: Consolidate kernel modules into common directory

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Viktor Malik <vmalik@redhat.com> 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@redhat.com>
>> ---
>> 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@redhat.com
>> ---
>>  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