[PATCH bpf-next] selftests/bpf: use auto-dependencies for test objects

[PATCH bpf-next] selftests/bpf: use auto-dependencies for test objects

[Ihor Solodrai]

Make use of -M compiler options when building .test.o objects to
generate .d files and avoid re-building all tests every time.

Previously, if a single test bpf program under selftests/bpf/progs/*.c
has changed, make would rebuild all the *.bpf.o, *.skel.h and *.test.o
objects, which is a lot of unnecessary work.

A typical dependency chain is:
progs/x.c -> x.bpf.o -> x.skel.h -> x.test.o -> trunner_binary

However for many tests it's not a 1:1 mapping by name, and so far
%.test.o have been simply dependent on all %.skel.h files, and
%.skel.h files on all %.bpf.o objects.

Avoid full rebuilds by instructing the compiler (via -MMD) to
produce *.d files with real dependencies, and appropriately including
them. Exploit make feature that rebuilds included makefiles if they
were changed by setting %.test.d as prerequisite for %.test.o files.

A couple of examples of compilation time speedup (after the first
clean build):

$ touch progs/verifier_and.c && time make -j8
Before: real	0m16.651s
After:  real	0m2.245s
$ touch progs/read_vsyscall.c && time make -j8
Before: real	0m15.743s
After:  real	0m1.575s

A drawback of this change is that now there is an overhead due to make
processing lots of .d files, which potentially may slow down unrelated
targets. However a time to make all from scratch hasn't changed
significantly:

$ make clean && time make -j8
Before: real	1m31.148s
After:  real	1m30.309s

Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
---
 tools/testing/selftests/bpf/.gitignore |  1 +
 tools/testing/selftests/bpf/Makefile   | 41 +++++++++++++++++++-------
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 5025401323af..4e4aae8aa7ec 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -31,6 +31,7 @@ test_tcp_check_syncookie_user
 test_sysctl
 xdping
 test_cpp
+*.d
 *.subskel.h
 *.skel.h
 *.lskel.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index dd49c1d23a60..95bb0b38d84b 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -477,7 +477,8 @@ xsk_xdp_progs.skel.h-deps := xsk_xdp_progs.bpf.o
 xdp_hw_metadata.skel.h-deps := xdp_hw_metadata.bpf.o
 xdp_features.skel.h-deps := xdp_features.bpf.o
 
-LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps)))
+LINKED_BPF_OBJS := $(foreach skel,$(LINKED_SKELS),$($(skel)-deps))
+LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(LINKED_BPF_OBJS))
 
 # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
 # $eval()) and pass control to DEFINE_TEST_RUNNER_RULES.
@@ -556,7 +557,11 @@ $(TRUNNER_BPF_LSKELS): %.lskel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.llinked3.o) name $$(notdir $$(<:.bpf.o=_lskel)) > $$@
 	$(Q)rm -f $$(<:.o=.llinked1.o) $$(<:.o=.llinked2.o) $$(<:.o=.llinked3.o)
 
-$(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
+$(LINKED_BPF_OBJS): %: $(TRUNNER_OUTPUT)/%
+
+# .SECONDEXPANSION here allows to correctly expand %-deps variables as prerequisites
+.SECONDEXPANSION:
+$(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_OUTPUT)/%: $$$$(%-deps) $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.bpf.o))
 	$(Q)$$(BPFTOOL) gen object $$(@:.skel.h=.linked1.o) $$(addprefix $(TRUNNER_OUTPUT)/,$$($$(@F)-deps))
 	$(Q)$$(BPFTOOL) gen object $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked1.o)
@@ -566,6 +571,14 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@
 	$(Q)$$(BPFTOOL) gen subskeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$(@:.skel.h=.subskel.h)
 	$(Q)rm -f $$(@:.skel.h=.linked1.o) $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked3.o)
+
+# When the compiler generates a %.d file, only skel basenames (not
+# full paths) are specified as prerequisites for corresponding %.o
+# file. This target makes %.skel.h basename dependent on full paths,
+# linking generated %.d dependency with actual %.skel.h files.
+$(notdir %.skel.h): $(TRUNNER_OUTPUT)/%.skel.h
+	@true
+
 endif
 
 # ensure we set up tests.h header generation rule just once
@@ -583,14 +596,20 @@ endif
 # Note: we cd into output directory to ensure embedded BPF object is found
 $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:			\
 		      $(TRUNNER_TESTS_DIR)/%.c				\
-		      $(TRUNNER_EXTRA_HDRS)				\
-		      $(TRUNNER_BPF_OBJS)				\
-		      $(TRUNNER_BPF_SKELS)				\
-		      $(TRUNNER_BPF_LSKELS)				\
-		      $(TRUNNER_BPF_SKELS_LINKED)			\
-		      $$(BPFOBJ) | $(TRUNNER_OUTPUT)
+		      $(TRUNNER_OUTPUT)/%.test.d
 	$$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
-	$(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
+	$(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -MMD -MT $$@ -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
+
+$(TRUNNER_TEST_OBJS:.o=.d): $(TRUNNER_OUTPUT)/%.test.d:			\
+			    $(TRUNNER_TESTS_DIR)/%.c			\
+			    $(TRUNNER_EXTRA_HDRS)			\
+			    $(TRUNNER_BPF_SKELS)			\
+			    $(TRUNNER_BPF_LSKELS)			\
+			    $(TRUNNER_BPF_SKELS_LINKED)			\
+			    $$(BPFOBJ) | $(TRUNNER_OUTPUT)
+
+include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
+
 
 $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
 		       %.c						\
@@ -768,8 +787,8 @@ $(OUTPUT)/uprobe_multi: uprobe_multi.c
 
 EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)			\
 	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
-	feature bpftool							\
-	$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h *.subskel.h	\
+	feature bpftool 						\
+	$(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h	\
 			       no_alu32 cpuv4 bpf_gcc bpf_testmod.ko	\
 			       bpf_test_no_cfi.ko			\
 			       liburandom_read.so)
-- 
2.34.1

Re: [PATCH bpf-next] selftests/bpf: use auto-dependencies for test objects

[Eduard Zingerman]

On Thu, 2024-07-11 at 21:09 +0000, Ihor Solodrai wrote:


[...]

> @@ -583,14 +596,20 @@ endif
>  # Note: we cd into output directory to ensure embedded BPF object is found
>  $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:			\
>  		      $(TRUNNER_TESTS_DIR)/%.c				\
> -		      $(TRUNNER_EXTRA_HDRS)				\
> -		      $(TRUNNER_BPF_OBJS)				\
> -		      $(TRUNNER_BPF_SKELS)				\
> -		      $(TRUNNER_BPF_LSKELS)				\
> -		      $(TRUNNER_BPF_SKELS_LINKED)			\
> -		      $$(BPFOBJ) | $(TRUNNER_OUTPUT)
> +		      $(TRUNNER_OUTPUT)/%.test.d
>  	$$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
> -	$(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
> +	$(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -MMD -MT $$@ -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)

Hi Ihor, nice patch, thank you for working on this!

While re-testing the patch I've noticed a strange behaviour:
$ cd <kernel>/tools/testing/selftests/bpf
$ git clean -xfd .
$ make -j14
$ touch progs/atomics.c 
$ make -j14 test_progs
  CLNG-BPF [test_maps] atomics.bpf.o
  CLNG-BPF [test_maps] atomics.bpf.o
  CLNG-BPF [test_maps] atomics.bpf.o
  GEN-SKEL [test_progs] atomics.lskel.h
  GEN-SKEL [test_progs-cpuv4] atomics.lskel.h
  GEN-SKEL [test_progs-no_alu32] atomics.lskel.h
  TEST-OBJ [test_progs] arena_htab.test.o
  TEST-OBJ [test_progs] atomics.test.o
  ... many lines ...
  TEST-OBJ [test_progs] uprobe_multi_test.test.o
  TEST-OBJ [test_progs] usdt.test.o
  TEST-OBJ [test_progs] verify_pkcs7_sig.test.o
  TEST-OBJ [test_progs] vmlinux.test.o
  TEST-OBJ [test_progs] xdp_adjust_tail.test.o
  TEST-OBJ [test_progs] xdp_metadata.test.o
  TEST-OBJ [test_progs] xdp_synproxy.test.o
  BINARY   test_progs
16:15:34 bpf$ make -j14 test_progs
  TEST-OBJ [test_progs] bpf_iter.test.o
  TEST-OBJ [test_progs] bpf_nf.test.o
  TEST-OBJ [test_progs] bpf_obj_id.test.o
  TEST-OBJ [test_progs] btf.test.o
  TEST-OBJ [test_progs] btf_write.test.o
  TEST-OBJ [test_progs] cgrp_local_storage.test.o
  TEST-OBJ [test_progs] iters.test.o
  TEST-OBJ [test_progs] lsm_cgroup.test.o
  TEST-OBJ [test_progs] send_signal.test.o
  TEST-OBJ [test_progs] sockmap_basic.test.o
  TEST-OBJ [test_progs] sockmap_listen.test.o
  TEST-OBJ [test_progs] trace_vprintk.test.o
  TEST-OBJ [test_progs] usdt.test.o
  TEST-OBJ [test_progs] xdp_metadata.test.o
  BINARY   test_progs
16:15:37 bpf$ make -j14 test_progs
  TEST-OBJ [test_progs] bpf_obj_id.test.o
  TEST-OBJ [test_progs] sockmap_listen.test.o
  TEST-OBJ [test_progs] xdp_metadata.test.o
  BINARY   test_progs
16:15:39 bpf$ make -j14 test_progs
  TEST-OBJ [test_progs] sockmap_listen.test.o
  BINARY   test_progs
16:15:41 bpf$ make -j14 test_progs
make: 'test_progs' is up to date.

Interestingly enough, this does not happen with your off-list version of
the patch shared this morning, the one with order-only dependency for .d:

  +$(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: $(TRUNNER_TESTS_DIR)/%.c | $(TRUNNER_OUTPUT)/%.test.d

Could you please double-check what's going on?

> +
> +$(TRUNNER_TEST_OBJS:.o=.d): $(TRUNNER_OUTPUT)/%.test.d:			\
> +			    $(TRUNNER_TESTS_DIR)/%.c			\
> +			    $(TRUNNER_EXTRA_HDRS)			\
> +			    $(TRUNNER_BPF_SKELS)			\
> +			    $(TRUNNER_BPF_LSKELS)			\
> +			    $(TRUNNER_BPF_SKELS_LINKED)			\
> +			    $$(BPFOBJ) | $(TRUNNER_OUTPUT)
> +
> +include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
> +
>  
>  $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
>  		       %.c						\

[...]

Re: [PATCH bpf-next] selftests/bpf: use auto-dependencies for test objects

[Eduard Zingerman]

On Thu, 2024-07-11 at 16:19 -0700, Eduard Zingerman wrote:

[...]


> While re-testing the patch I've noticed a strange behaviour:
> $ cd <kernel>/tools/testing/selftests/bpf
> $ git clean -xfd .
> $ make -j14
> $ touch progs/atomics.c 
> $ make -j14 test_progs
>   CLNG-BPF [test_maps] atomics.bpf.o
>   CLNG-BPF [test_maps] atomics.bpf.o
>   CLNG-BPF [test_maps] atomics.bpf.o
>   GEN-SKEL [test_progs] atomics.lskel.h
>   GEN-SKEL [test_progs-cpuv4] atomics.lskel.h
>   GEN-SKEL [test_progs-no_alu32] atomics.lskel.h
>   TEST-OBJ [test_progs] arena_htab.test.o
>   TEST-OBJ [test_progs] atomics.test.o
>   ... many lines ...
>   TEST-OBJ [test_progs] uprobe_multi_test.test.o
>   TEST-OBJ [test_progs] usdt.test.o
>   TEST-OBJ [test_progs] verify_pkcs7_sig.test.o
>   TEST-OBJ [test_progs] vmlinux.test.o
>   TEST-OBJ [test_progs] xdp_adjust_tail.test.o
>   TEST-OBJ [test_progs] xdp_metadata.test.o
>   TEST-OBJ [test_progs] xdp_synproxy.test.o
>   BINARY   test_progs
> 16:15:34 bpf$ make -j14 test_progs
>   TEST-OBJ [test_progs] bpf_iter.test.o
>   TEST-OBJ [test_progs] bpf_nf.test.o
>   TEST-OBJ [test_progs] bpf_obj_id.test.o
>   TEST-OBJ [test_progs] btf.test.o
>   TEST-OBJ [test_progs] btf_write.test.o
>   TEST-OBJ [test_progs] cgrp_local_storage.test.o
>   TEST-OBJ [test_progs] iters.test.o
>   TEST-OBJ [test_progs] lsm_cgroup.test.o
>   TEST-OBJ [test_progs] send_signal.test.o
>   TEST-OBJ [test_progs] sockmap_basic.test.o
>   TEST-OBJ [test_progs] sockmap_listen.test.o
>   TEST-OBJ [test_progs] trace_vprintk.test.o
>   TEST-OBJ [test_progs] usdt.test.o
>   TEST-OBJ [test_progs] xdp_metadata.test.o
>   BINARY   test_progs
> 16:15:37 bpf$ make -j14 test_progs
>   TEST-OBJ [test_progs] bpf_obj_id.test.o
>   TEST-OBJ [test_progs] sockmap_listen.test.o
>   TEST-OBJ [test_progs] xdp_metadata.test.o
>   BINARY   test_progs
> 16:15:39 bpf$ make -j14 test_progs
>   TEST-OBJ [test_progs] sockmap_listen.test.o
>   BINARY   test_progs
> 16:15:41 bpf$ make -j14 test_progs
> make: 'test_progs' is up to date.
> 
> Interestingly enough, this does not happen with your off-list version of
> the patch shared this morning, the one with order-only dependency for .d:
> 
>   +$(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: $(TRUNNER_TESTS_DIR)/%.c | $(TRUNNER_OUTPUT)/%.test.d
> 
> Could you please double-check what's going on?

After some investigation it turned out that behaviour is specific to LLVM.
Under certain not yet clear conditions clang writes .d file *after* writing .o file.
For example:

{llvm} 19:15:59 bpf$ rm ringbuf.test.o; make `pwd`/ringbuf.test.o; ls -l --time-style=full-iso `pwd`/ringbuf.test.{o,d}
  TEST-OBJ [test_progs] ringbuf.test.o
-rw-rw-r-- 1 eddy eddy   1947 2024-07-11 19:16:01.059016929 -0700 /home/eddy/work/bpf-next/tools/testing/selftests/bpf/ringbuf.test.d
-rw-rw-r-- 1 eddy eddy 160544 2024-07-11 19:16:01.055016909 -0700 /home/eddy/work/bpf-next/tools/testing/selftests/bpf/ringbuf.test.o

Note that ringbuf.test.d is newer than ringbuf.test.o.
This happens on each 10 or 20 run of the command.
Such behaviour clearly defies the reason for dependency files generation.

The decision for now is to avoid specifying .d files as direct dependencies
of the .o files and use order-only dependencies instead.
The make feature for reloading included makefiles would take care
of correctly re-specifying dependencies.

[...]

Re: [PATCH bpf-next] selftests/bpf: use auto-dependencies for test objects

[Ihor Solodrai]

On Thursday, July 11th, 2024 at 7:21 PM, Eduard Zingerman <eddyz87@gmail.com> wrote:

[...]

> After some investigation it turned out that behaviour is specific to LLVM.
> Under certain not yet clear conditions clang writes .d file after writing .o file.
> For example:
> 
> {llvm} 19:15:59 bpf$ rm ringbuf.test.o; make `pwd`/ringbuf.test.o; ls -l --time-style=full-iso `pwd`/ringbuf.test.{o,d}
> TEST-OBJ [test_progs] ringbuf.test.o
> -rw-rw-r-- 1 eddy eddy 1947 2024-07-11 19:16:01.059016929 -0700 /home/eddy/work/bpf-next/tools/testing/selftests/bpf/ringbuf.test.d
> -rw-rw-r-- 1 eddy eddy 160544 2024-07-11 19:16:01.055016909 -0700 /home/eddy/work/bpf-next/tools/testing/selftests/bpf/ringbuf.test.o
> 
> Note that ringbuf.test.d is newer than ringbuf.test.o.
> This happens on each 10 or 20 run of the command.
> Such behaviour clearly defies the reason for dependency files generation.
> 
> The decision for now is to avoid specifying .d files as direct dependencies
> of the .o files and use order-only dependencies instead.
> The make feature for reloading included makefiles would take care
> of correctly re-specifying dependencies.

Eduard, thank you for testing.

I was able to reproduce the problem you've noticed using LLVM 19.

As far as I understand, direct dependency is not reliable here because
it implicitly expects %.d files to always be older than %.o files.

There are two possible situations when a given %.test.o must be built.

If %.test.d does not exist, then the include will be empty.  However,
because there is a target for %.test.d and a dependency on it, all the
%.bpf.o and skels will be built. And by the time CC %.test.o happens,
all its dependencies are ready. This is true for %.test.d both as
direct and order only dependency.

If %.test.d exists, then make included it and there is an additional
target for a particular %.test.o with concrete dependencies, which are
built as necessary. And the explicit %.test.d doesn't trigger, because
the file is up-to-date (which is exactly what we want).

In the second case, if %.test.d prerequisite is not order only, and
%.test.d sometimes happens to be newer than %.test.o (this is the case
for clang, but not for gcc), make would run CC again, which may update
%.d again and create a loop.

I think making %.test.d an order only prerequisite is the right fix
here, because we clearly can not expect that all compiler versions
will output %.d before %.o (even though it makes sense).

I will send v2 of the patch shortly.