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

[PATCH bpf-next v4] 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>

---
v3 -> v4: Make $(TRUNNER_BPF_OBJS) order only prereq of trunner binary
v2 -> v3: Restore dependency on $(TRUNNER_BPF_OBJS)
v1 -> v2: Make %.test.d prerequisite order only
---
 tools/testing/selftests/bpf/.gitignore |  1 +
 tools/testing/selftests/bpf/Makefile   | 44 +++++++++++++++++++-------
 2 files changed, 34 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..66478446af9d 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						\
@@ -608,6 +627,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
 	$(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
 endif
 
+# some X.test.o files have runtime dependencies on Y.bpf.o files
+$(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
+
 $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
 			     $(TRUNNER_EXTRA_OBJS) $$(BPFOBJ)		\
 			     $(RESOLVE_BTFIDS)				\
@@ -768,8 +790,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 v4] selftests/bpf: use auto-dependencies for test objects

[Andrii Nakryiko]

On Thu, Jul 18, 2024 at 3:57 PM Ihor Solodrai <ihor.solodrai@pm.me> wrote:
>
> 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>
>
> ---
> v3 -> v4: Make $(TRUNNER_BPF_OBJS) order only prereq of trunner binary
> v2 -> v3: Restore dependency on $(TRUNNER_BPF_OBJS)
> v1 -> v2: Make %.test.d prerequisite order only
> ---
>  tools/testing/selftests/bpf/.gitignore |  1 +
>  tools/testing/selftests/bpf/Makefile   | 44 +++++++++++++++++++-------
>  2 files changed, 34 insertions(+), 11 deletions(-)
>

It seems to behave correctly, but it reports wrong flavor when
building .bpf.o, e.g.,:

$ touch progs/test_vmlinux.c
$ make -j90
  CLNG-BPF [test_maps] test_vmlinux.bpf.o
  CLNG-BPF [test_maps] test_vmlinux.bpf.o
  CLNG-BPF [test_maps] test_vmlinux.bpf.o
  GEN-SKEL [test_progs] test_vmlinux.skel.h
  GEN-SKEL [test_progs-cpuv4] test_vmlinux.skel.h
  GEN-SKEL [test_progs-no_alu32] test_vmlinux.skel.h
  TEST-OBJ [test_progs] vmlinux.test.o
  TEST-OBJ [test_progs-no_alu32] vmlinux.test.o
  EXT-COPY [test_progs-no_alu32] urandom_read bpf_testmod.ko
bpf_test_no_cfi.ko liburandom_read.so xdp_synproxy sign-file
uprobe_multi ima_setup.sh verify_sig_setup.sh
btf_dump_test_case_bitfields.c btf_dump_test_case_multidim.c
btf_dump_test_case_namespacing.c btf_dump_test_case_ordering.c
btf_dump_test_case_packing.c btf_dump_test_case_padding.c
btf_dump_test_case_syntax.c
  TEST-OBJ [test_progs-cpuv4] vmlinux.test.o
  EXT-COPY [test_progs-cpuv4] urandom_read bpf_testmod.ko
bpf_test_no_cfi.ko liburandom_read.so xdp_synproxy sign-file
uprobe_multi ima_setup.sh verify_sig_setup.sh
btf_dump_test_case_bitfields.c btf_dump_test_case_multidim.c
btf_dump_test_case_namespacing.c btf_dump_test_case_ordering.c
btf_dump_test_case_packing.c btf_dump_test_case_padding.c
btf_dump_test_case_syntax.c
make[1]: Nothing to be done for 'docs'.
  BINARY   test_progs
  BINARY   test_progs-no_alu32
  BINARY   test_progs-cpuv4
$ ls -la test_vmlinux.bpf.o no_alu32/test_vmlinux.bpf.o cpuv4/test_vmlinux.bpf.o
-rw-r--r-- 1 andriin users 21344 Jul 19 11:08 cpuv4/test_vmlinux.bpf.o
-rw-r--r-- 1 andriin users 21408 Jul 19 11:08 no_alu32/test_vmlinux.bpf.o
-rw-r--r-- 1 andriin users 21408 Jul 19 11:08 test_vmlinux.bpf.o


Note [test_maps] for all three variants (I expected
test_maps/test_progs + no_alu32 + cpuv4, just like we see for skel.h).
Can you please double check what's going on? Looking at timestamps it
seems like they are actually regenerated, though.


BTW, if you get a chance, see if you can avoid unnecessary EXT-COPY as
well (probably a bit smarter rule dependency should be set up, e.g.,
phony target that then depends on actual files or something like
that).

Regardless, this is a massive improvement and seems to work correctly,
so I've applied this and will wait for follow ups. Thanks a lot!

BTW, are you planning to look into vmlinux.h optimization as well?

> 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

[...]

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

[Ihor Solodrai]

On Friday, July 19th, 2024 at 11:18 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

[...]

> Note [test_maps] for all three variants (I expected
> test_maps/test_progs + no_alu32 + cpuv4, just like we see for skel.h).
> Can you please double check what's going on? Looking at timestamps it
> seems like they are actually regenerated, though.

Yeah, this is weird. Will look into it.


> BTW, if you get a chance, see if you can avoid unnecessary EXT-COPY as
> well (probably a bit smarter rule dependency should be set up, e.g.,
> phony target that then depends on actual files or something like
> that).
> 
> Regardless, this is a massive improvement and seems to work correctly,
> so I've applied this and will wait for follow ups. Thanks a lot!

You're welcome! Happy to see my first patch accepted!

> 
> BTW, are you planning to look into vmlinux.h optimization as well?

Yes, it seems there is more room for improvements.

I think making changes like this patch is a great way to get familiar
with the codebase, which is what I'm trying to do. Even better if the
changes are actually useful :)

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

[Tony Ambardar]

On Fri, Jul 19, 2024 at 11:18:16AM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 18, 2024 at 3:57 PM Ihor Solodrai <ihor.solodrai@pm.me> wrote:
> >
> > 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>
> >
> > ---
> > v3 -> v4: Make $(TRUNNER_BPF_OBJS) order only prereq of trunner binary
> > v2 -> v3: Restore dependency on $(TRUNNER_BPF_OBJS)
> > v1 -> v2: Make %.test.d prerequisite order only
> > ---
> >  tools/testing/selftests/bpf/.gitignore |  1 +
> >  tools/testing/selftests/bpf/Makefile   | 44 +++++++++++++++++++-------
> >  2 files changed, 34 insertions(+), 11 deletions(-)
> >
> 
> It seems to behave correctly, but it reports wrong flavor when
> building .bpf.o, e.g.,:
> 
Hi Andrii,

This is actually an old, confusing bug unrelated to the current (very
nice) improvements. I have a fix as part of a larger series
targeting libc portability and MIPS support which I'll post shortly.  Or
I can send separately if you like?

Thanks,
Tony

> $ touch progs/test_vmlinux.c
> $ make -j90
>   CLNG-BPF [test_maps] test_vmlinux.bpf.o
>   CLNG-BPF [test_maps] test_vmlinux.bpf.o
>   CLNG-BPF [test_maps] test_vmlinux.bpf.o
>   GEN-SKEL [test_progs] test_vmlinux.skel.h
>   GEN-SKEL [test_progs-cpuv4] test_vmlinux.skel.h
>   GEN-SKEL [test_progs-no_alu32] test_vmlinux.skel.h
>   TEST-OBJ [test_progs] vmlinux.test.o
>   TEST-OBJ [test_progs-no_alu32] vmlinux.test.o
>   EXT-COPY [test_progs-no_alu32] urandom_read bpf_testmod.ko
> bpf_test_no_cfi.ko liburandom_read.so xdp_synproxy sign-file
> uprobe_multi ima_setup.sh verify_sig_setup.sh
> btf_dump_test_case_bitfields.c btf_dump_test_case_multidim.c
> btf_dump_test_case_namespacing.c btf_dump_test_case_ordering.c
> btf_dump_test_case_packing.c btf_dump_test_case_padding.c
> btf_dump_test_case_syntax.c
>   TEST-OBJ [test_progs-cpuv4] vmlinux.test.o
>   EXT-COPY [test_progs-cpuv4] urandom_read bpf_testmod.ko
> bpf_test_no_cfi.ko liburandom_read.so xdp_synproxy sign-file
> uprobe_multi ima_setup.sh verify_sig_setup.sh
> btf_dump_test_case_bitfields.c btf_dump_test_case_multidim.c
> btf_dump_test_case_namespacing.c btf_dump_test_case_ordering.c
> btf_dump_test_case_packing.c btf_dump_test_case_padding.c
> btf_dump_test_case_syntax.c
> make[1]: Nothing to be done for 'docs'.
>   BINARY   test_progs
>   BINARY   test_progs-no_alu32
>   BINARY   test_progs-cpuv4
> $ ls -la test_vmlinux.bpf.o no_alu32/test_vmlinux.bpf.o cpuv4/test_vmlinux.bpf.o
> -rw-r--r-- 1 andriin users 21344 Jul 19 11:08 cpuv4/test_vmlinux.bpf.o
> -rw-r--r-- 1 andriin users 21408 Jul 19 11:08 no_alu32/test_vmlinux.bpf.o
> -rw-r--r-- 1 andriin users 21408 Jul 19 11:08 test_vmlinux.bpf.o
> 
> 
> Note [test_maps] for all three variants (I expected
> test_maps/test_progs + no_alu32 + cpuv4, just like we see for skel.h).
> Can you please double check what's going on? Looking at timestamps it
> seems like they are actually regenerated, though.
> 
> 
> BTW, if you get a chance, see if you can avoid unnecessary EXT-COPY as
> well (probably a bit smarter rule dependency should be set up, e.g.,
> phony target that then depends on actual files or something like
> that).
> 
> Regardless, this is a massive improvement and seems to work correctly,
> so I've applied this and will wait for follow ups. Thanks a lot!
> 
> BTW, are you planning to look into vmlinux.h optimization as well?
> 
> > 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
> 
> [...]

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

[Andrii Nakryiko]

On Fri, Jul 19, 2024 at 2:54 PM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> On Fri, Jul 19, 2024 at 11:18:16AM -0700, Andrii Nakryiko wrote:
> > On Thu, Jul 18, 2024 at 3:57 PM Ihor Solodrai <ihor.solodrai@pm.me> wrote:
> > >
> > > 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>
> > >
> > > ---
> > > v3 -> v4: Make $(TRUNNER_BPF_OBJS) order only prereq of trunner binary
> > > v2 -> v3: Restore dependency on $(TRUNNER_BPF_OBJS)
> > > v1 -> v2: Make %.test.d prerequisite order only
> > > ---
> > >  tools/testing/selftests/bpf/.gitignore |  1 +
> > >  tools/testing/selftests/bpf/Makefile   | 44 +++++++++++++++++++-------
> > >  2 files changed, 34 insertions(+), 11 deletions(-)
> > >
> >
> > It seems to behave correctly, but it reports wrong flavor when
> > building .bpf.o, e.g.,:
> >
> Hi Andrii,
>
> This is actually an old, confusing bug unrelated to the current (very
> nice) improvements. I have a fix as part of a larger series
> targeting libc portability and MIPS support which I'll post shortly.  Or
> I can send separately if you like?

Please send it separately, having small targeted Makefile changes
makes it much easier to test, review, and land them quickly.

>
> Thanks,
> Tony
>
> > $ touch progs/test_vmlinux.c
> > $ make -j90
> >   CLNG-BPF [test_maps] test_vmlinux.bpf.o
> >   CLNG-BPF [test_maps] test_vmlinux.bpf.o
> >   CLNG-BPF [test_maps] test_vmlinux.bpf.o
> >   GEN-SKEL [test_progs] test_vmlinux.skel.h
> >   GEN-SKEL [test_progs-cpuv4] test_vmlinux.skel.h
> >   GEN-SKEL [test_progs-no_alu32] test_vmlinux.skel.h
> >   TEST-OBJ [test_progs] vmlinux.test.o
> >   TEST-OBJ [test_progs-no_alu32] vmlinux.test.o
> >   EXT-COPY [test_progs-no_alu32] urandom_read bpf_testmod.ko
> > bpf_test_no_cfi.ko liburandom_read.so xdp_synproxy sign-file
> > uprobe_multi ima_setup.sh verify_sig_setup.sh
> > btf_dump_test_case_bitfields.c btf_dump_test_case_multidim.c
> > btf_dump_test_case_namespacing.c btf_dump_test_case_ordering.c
> > btf_dump_test_case_packing.c btf_dump_test_case_padding.c
> > btf_dump_test_case_syntax.c
> >   TEST-OBJ [test_progs-cpuv4] vmlinux.test.o
> >   EXT-COPY [test_progs-cpuv4] urandom_read bpf_testmod.ko
> > bpf_test_no_cfi.ko liburandom_read.so xdp_synproxy sign-file
> > uprobe_multi ima_setup.sh verify_sig_setup.sh
> > btf_dump_test_case_bitfields.c btf_dump_test_case_multidim.c
> > btf_dump_test_case_namespacing.c btf_dump_test_case_ordering.c
> > btf_dump_test_case_packing.c btf_dump_test_case_padding.c
> > btf_dump_test_case_syntax.c
> > make[1]: Nothing to be done for 'docs'.
> >   BINARY   test_progs
> >   BINARY   test_progs-no_alu32
> >   BINARY   test_progs-cpuv4
> > $ ls -la test_vmlinux.bpf.o no_alu32/test_vmlinux.bpf.o cpuv4/test_vmlinux.bpf.o
> > -rw-r--r-- 1 andriin users 21344 Jul 19 11:08 cpuv4/test_vmlinux.bpf.o
> > -rw-r--r-- 1 andriin users 21408 Jul 19 11:08 no_alu32/test_vmlinux.bpf.o
> > -rw-r--r-- 1 andriin users 21408 Jul 19 11:08 test_vmlinux.bpf.o
> >
> >
> > Note [test_maps] for all three variants (I expected
> > test_maps/test_progs + no_alu32 + cpuv4, just like we see for skel.h).
> > Can you please double check what's going on? Looking at timestamps it
> > seems like they are actually regenerated, though.
> >
> >
> > BTW, if you get a chance, see if you can avoid unnecessary EXT-COPY as
> > well (probably a bit smarter rule dependency should be set up, e.g.,
> > phony target that then depends on actual files or something like
> > that).
> >
> > Regardless, this is a massive improvement and seems to work correctly,
> > so I've applied this and will wait for follow ups. Thanks a lot!
> >
> > BTW, are you planning to look into vmlinux.h optimization as well?
> >
> > > 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
> >
> > [...]

[PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[Tony Ambardar]

Make log output incorrectly shows 'test_maps' as the binary name for every
'CLNG-BPF' build step, apparently picking up the last value defined for the
$(TRUNNER_BINARY) variable. Update the 'CLANG_BPF_BUILD_RULE' variants to
fix this confusing output.

Current output:
  CLNG-BPF [test_maps] access_map_in_map.bpf.o
  GEN-SKEL [test_progs] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_maps] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_maps] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h

After fix:
  CLNG-BPF [test_progs] access_map_in_map.bpf.o
  GEN-SKEL [test_progs] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_progs-no_alu32] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_progs-cpuv4] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h

Fixes: a5d0c26a2784 ("selftests/bpf: Add a cpuv4 test runner for cpu=v4 testing")
Fixes: 89ad7420b25c ("selftests/bpf: Drop the need for LLVM's llc")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/Makefile | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 0b4bfbc0ef68..67921e3367dd 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -425,27 +425,28 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
 $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
 
 # Build BPF object using Clang
-# $1 - input .c file
-# $2 - output .o file
-# $3 - CFLAGS
+# $1 - binary name
+# $2 - input .c file
+# $3 - output .o file
+# $4 - CFLAGS
 define CLANG_BPF_BUILD_RULE
-	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v3 -o $2
+	$(call msg,CLNG-BPF,$1,$3)
+	$(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v3 -o $3
 endef
 # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
 define CLANG_NOALU32_BPF_BUILD_RULE
-	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v2 -o $2
+	$(call msg,CLNG-BPF,$1,$3)
+	$(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v2 -o $3
 endef
 # Similar to CLANG_BPF_BUILD_RULE, but with cpu-v4
 define CLANG_CPUV4_BPF_BUILD_RULE
-	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v4 -o $2
+	$(call msg,CLNG-BPF,$1,$3)
+	$(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v4 -o $3
 endef
 # Build BPF object using GCC
 define GCC_BPF_BUILD_RULE
-	$(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(BPF_GCC) $3 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $1 -o $2
+	$(call msg,GCC-BPF,$1,$3)
+	$(Q)$(BPF_GCC) $4 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $2 -o $3
 endef
 
 SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
@@ -534,7 +535,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:				\
 		     $(wildcard $(BPFDIR)/bpf_*.h)			\
 		     $(wildcard $(BPFDIR)/*.bpf.h)			\
 		     | $(TRUNNER_OUTPUT) $$(BPFOBJ)
-	$$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,			\
+	$$(call $(TRUNNER_BPF_BUILD_RULE),$(TRUNNER_BINARY),$$<,$$@,	\
 					  $(TRUNNER_BPF_CFLAGS)         \
 					  $$($$<-CFLAGS)		\
 					  $$($$<-$2-CFLAGS))
-- 
2.34.1

Re: [PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

Dear patch submitter,

CI has tested the following submission:
Status:     SUCCESS
Name:       [bpf-next,v1] selftests/bpf: Fix wrong binary in Makefile log output
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872629&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10015585896

No further action is necessary on your part.


Please note: this email is coming from an unmonitored mailbox. If you have
questions or feedback, please reach out to the Meta Kernel CI team at
kernel-ci@meta.com.

Re: [PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[Eduard Zingerman]

On Fri, 2024-07-19 at 16:21 -0700, Tony Ambardar wrote:

[...]


>  define GCC_BPF_BUILD_RULE
> -	$(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
> -	$(Q)$(BPF_GCC) $3 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $1 -o $2
> +	$(call msg,GCC-BPF,$1,$3)
> +	$(Q)$(BPF_GCC) $4 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $2 -o $3
>  endef
>  
>  SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
> @@ -534,7 +535,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:				\
>  		     $(wildcard $(BPFDIR)/bpf_*.h)			\
>  		     $(wildcard $(BPFDIR)/*.bpf.h)			\
>  		     | $(TRUNNER_OUTPUT) $$(BPFOBJ)
> -	$$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,			\
> +	$$(call $(TRUNNER_BPF_BUILD_RULE),$(TRUNNER_BINARY),$$<,$$@,	\

At first I found it quite confusing that we use TRUNNER_BINARY variable in this define,
but can't use it in the CLANG_BPF_BUILD_RULE and co.
However, it looks like this is because of the eval in the (eval (call ...)) pair,
used to invoke DEFINE_TEST_RUNNER_RULES.

Suggested patch works and is probably the simplest fix.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

>  					  $(TRUNNER_BPF_CFLAGS)         \
>  					  $$($$<-CFLAGS)		\
>  					  $$($$<-$2-CFLAGS))

Re: [PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[Andrii Nakryiko]

On Fri, Jul 19, 2024 at 4:22 PM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> Make log output incorrectly shows 'test_maps' as the binary name for every
> 'CLNG-BPF' build step, apparently picking up the last value defined for the
> $(TRUNNER_BINARY) variable. Update the 'CLANG_BPF_BUILD_RULE' variants to
> fix this confusing output.
>
> Current output:
>   CLNG-BPF [test_maps] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs] access_map_in_map.skel.h
>   ...
>   CLNG-BPF [test_maps] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
>   ...
>   CLNG-BPF [test_maps] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h
>
> After fix:
>   CLNG-BPF [test_progs] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs] access_map_in_map.skel.h
>   ...
>   CLNG-BPF [test_progs-no_alu32] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
>   ...
>   CLNG-BPF [test_progs-cpuv4] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h
>
> Fixes: a5d0c26a2784 ("selftests/bpf: Add a cpuv4 test runner for cpu=v4 testing")
> Fixes: 89ad7420b25c ("selftests/bpf: Drop the need for LLVM's llc")
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 0b4bfbc0ef68..67921e3367dd 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -425,27 +425,28 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
>  $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
>
>  # Build BPF object using Clang
> -# $1 - input .c file
> -# $2 - output .o file
> -# $3 - CFLAGS
> +# $1 - binary name
> +# $2 - input .c file
> +# $3 - output .o file
> +# $4 - CFLAGS
>  define CLANG_BPF_BUILD_RULE
> -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v3 -o $2
> +       $(call msg,CLNG-BPF,$1,$3)
> +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v3 -o $3

this works, but did you have to renumber all parameters? Let's maybe
pass this binary name as the 4th argument?

pw-bot: cr

>  endef
>  # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
>  define CLANG_NOALU32_BPF_BUILD_RULE
> -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v2 -o $2
> +       $(call msg,CLNG-BPF,$1,$3)
> +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v2 -o $3
>  endef
>  # Similar to CLANG_BPF_BUILD_RULE, but with cpu-v4
>  define CLANG_CPUV4_BPF_BUILD_RULE
> -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v4 -o $2
> +       $(call msg,CLNG-BPF,$1,$3)
> +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v4 -o $3
>  endef
>  # Build BPF object using GCC
>  define GCC_BPF_BUILD_RULE
> -       $(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
> -       $(Q)$(BPF_GCC) $3 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $1 -o $2
> +       $(call msg,GCC-BPF,$1,$3)
> +       $(Q)$(BPF_GCC) $4 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $2 -o $3
>  endef
>
>  SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
> @@ -534,7 +535,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:                             \
>                      $(wildcard $(BPFDIR)/bpf_*.h)                      \
>                      $(wildcard $(BPFDIR)/*.bpf.h)                      \
>                      | $(TRUNNER_OUTPUT) $$(BPFOBJ)
> -       $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,                      \
> +       $$(call $(TRUNNER_BPF_BUILD_RULE),$(TRUNNER_BINARY),$$<,$$@,    \
>                                           $(TRUNNER_BPF_CFLAGS)         \
>                                           $$($$<-CFLAGS)                \
>                                           $$($$<-$2-CFLAGS))
> --
> 2.34.1
>

Re: [PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[Tony Ambardar]

On Fri, Jul 19, 2024 at 07:57:09PM -0700, Andrii Nakryiko wrote:
> On Fri, Jul 19, 2024 at 4:22 PM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > Make log output incorrectly shows 'test_maps' as the binary name for every
> > 'CLNG-BPF' build step, apparently picking up the last value defined for the
> > $(TRUNNER_BINARY) variable. Update the 'CLANG_BPF_BUILD_RULE' variants to
> > fix this confusing output.
> >
> > Current output:
> >   CLNG-BPF [test_maps] access_map_in_map.bpf.o
> >   GEN-SKEL [test_progs] access_map_in_map.skel.h
> >   ...
> >   CLNG-BPF [test_maps] access_map_in_map.bpf.o
> >   GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
> >   ...
> >   CLNG-BPF [test_maps] access_map_in_map.bpf.o
> >   GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h
> >
> > After fix:
> >   CLNG-BPF [test_progs] access_map_in_map.bpf.o
> >   GEN-SKEL [test_progs] access_map_in_map.skel.h
> >   ...
> >   CLNG-BPF [test_progs-no_alu32] access_map_in_map.bpf.o
> >   GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
> >   ...
> >   CLNG-BPF [test_progs-cpuv4] access_map_in_map.bpf.o
> >   GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h
> >
> > Fixes: a5d0c26a2784 ("selftests/bpf: Add a cpuv4 test runner for cpu=v4 testing")
> > Fixes: 89ad7420b25c ("selftests/bpf: Drop the need for LLVM's llc")
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/Makefile | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 0b4bfbc0ef68..67921e3367dd 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -425,27 +425,28 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
> >  $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
> >
> >  # Build BPF object using Clang
> > -# $1 - input .c file
> > -# $2 - output .o file
> > -# $3 - CFLAGS
> > +# $1 - binary name
> > +# $2 - input .c file
> > +# $3 - output .o file
> > +# $4 - CFLAGS
> >  define CLANG_BPF_BUILD_RULE
> > -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> > -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v3 -o $2
> > +       $(call msg,CLNG-BPF,$1,$3)
> > +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v3 -o $3
> 
> this works, but did you have to renumber all parameters? Let's maybe
> pass this binary name as the 4th argument?
> 

OK, I'll update as requested and minimize the diff.

> pw-bot: cr
> 
> >  endef
> >  # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
> >  define CLANG_NOALU32_BPF_BUILD_RULE
> > -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> > -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v2 -o $2
> > +       $(call msg,CLNG-BPF,$1,$3)
> > +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v2 -o $3
> >  endef
> >  # Similar to CLANG_BPF_BUILD_RULE, but with cpu-v4
> >  define CLANG_CPUV4_BPF_BUILD_RULE
> > -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> > -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v4 -o $2
> > +       $(call msg,CLNG-BPF,$1,$3)
> > +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v4 -o $3
> >  endef
> >  # Build BPF object using GCC
> >  define GCC_BPF_BUILD_RULE
> > -       $(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
> > -       $(Q)$(BPF_GCC) $3 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $1 -o $2
> > +       $(call msg,GCC-BPF,$1,$3)
> > +       $(Q)$(BPF_GCC) $4 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $2 -o $3
> >  endef
> >
> >  SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
> > @@ -534,7 +535,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:                             \
> >                      $(wildcard $(BPFDIR)/bpf_*.h)                      \
> >                      $(wildcard $(BPFDIR)/*.bpf.h)                      \
> >                      | $(TRUNNER_OUTPUT) $$(BPFOBJ)
> > -       $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,                      \
> > +       $$(call $(TRUNNER_BPF_BUILD_RULE),$(TRUNNER_BINARY),$$<,$$@,    \
> >                                           $(TRUNNER_BPF_CFLAGS)         \
> >                                           $$($$<-CFLAGS)                \
> >                                           $$($$<-$2-CFLAGS))
> > --
> > 2.34.1
> >

Re: [PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[Tony Ambardar]

On Fri, Jul 19, 2024 at 10:23:33PM -0700, Tony Ambardar wrote:
> On Fri, Jul 19, 2024 at 07:57:09PM -0700, Andrii Nakryiko wrote:
> > On Fri, Jul 19, 2024 at 4:22 PM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> > >

[...]

> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -425,27 +425,28 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
> > >  $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
> > >
> > >  # Build BPF object using Clang
> > > -# $1 - input .c file
> > > -# $2 - output .o file
> > > -# $3 - CFLAGS
> > > +# $1 - binary name
> > > +# $2 - input .c file
> > > +# $3 - output .o file
> > > +# $4 - CFLAGS
> > >  define CLANG_BPF_BUILD_RULE
> > > -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> > > -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v3 -o $2
> > > +       $(call msg,CLNG-BPF,$1,$3)
> > > +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v3 -o $3
> > 
> > this works, but did you have to renumber all parameters? Let's maybe
> > pass this binary name as the 4th argument?
> > 
> 
> OK, I'll update as requested and minimize the diff.
> 
Hi Andrii,

I sent out a v2 based on your suggestions but omitted Eduard's Acked-by:
by mistake. Should I resubmit or is that something you can update?

Thanks,
Tony
> > pw-bot: cr
> > 
> > >  endef
> > >  # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
> > >  define CLANG_NOALU32_BPF_BUILD_RULE
> > > -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> > > -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v2 -o $2
> > > +       $(call msg,CLNG-BPF,$1,$3)
> > > +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v2 -o $3
> > >  endef
> > >  # Similar to CLANG_BPF_BUILD_RULE, but with cpu-v4
> > >  define CLANG_CPUV4_BPF_BUILD_RULE
> > > -       $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
> > > -       $(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v4 -o $2
> > > +       $(call msg,CLNG-BPF,$1,$3)
> > > +       $(Q)$(CLANG) $4 -O2 --target=bpf -c $2 -mcpu=v4 -o $3
> > >  endef
> > >  # Build BPF object using GCC
> > >  define GCC_BPF_BUILD_RULE
> > > -       $(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
> > > -       $(Q)$(BPF_GCC) $3 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $1 -o $2
> > > +       $(call msg,GCC-BPF,$1,$3)
> > > +       $(Q)$(BPF_GCC) $4 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $2 -o $3
> > >  endef
> > >
> > >  SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
> > > @@ -534,7 +535,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:                             \
> > >                      $(wildcard $(BPFDIR)/bpf_*.h)                      \
> > >                      $(wildcard $(BPFDIR)/*.bpf.h)                      \
> > >                      | $(TRUNNER_OUTPUT) $$(BPFOBJ)
> > > -       $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,                      \
> > > +       $$(call $(TRUNNER_BPF_BUILD_RULE),$(TRUNNER_BINARY),$$<,$$@,    \
> > >                                           $(TRUNNER_BPF_CFLAGS)         \
> > >                                           $$($$<-CFLAGS)                \
> > >                                           $$($$<-$2-CFLAGS))
> > > --
> > > 2.34.1
> > >

Re: [PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[Eduard Zingerman]

On Mon, 2024-07-22 at 18:35 -0700, Tony Ambardar wrote:
[...]

> Hi Andrii,
> 
> I sent out a v2 based on your suggestions but omitted Eduard's Acked-by:
> by mistake. Should I resubmit or is that something you can update?

That's fine, don't bother.

[...]

Re: [PATCH bpf-next v1] selftests/bpf: Fix wrong binary in Makefile log output

[Andrii Nakryiko]

On Mon, Jul 22, 2024 at 6:37 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2024-07-22 at 18:35 -0700, Tony Ambardar wrote:
> [...]
>
> > Hi Andrii,
> >
> > I sent out a v2 based on your suggestions but omitted Eduard's Acked-by:
> > by mistake. Should I resubmit or is that something you can update?
>
> That's fine, don't bother.
>

Added it while applying.

> [...]
>

[PATCH bpf-next v2] selftests/bpf: Fix wrong binary in Makefile log output

[Tony Ambardar]

Make log output incorrectly shows 'test_maps' as the binary name for every
'CLNG-BPF' build step, apparently picking up the last value defined for the
$(TRUNNER_BINARY) variable. Update the 'CLANG_BPF_BUILD_RULE' variants to
fix this confusing output.

Current output:
  CLNG-BPF [test_maps] access_map_in_map.bpf.o
  GEN-SKEL [test_progs] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_maps] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_maps] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h

After fix:
  CLNG-BPF [test_progs] access_map_in_map.bpf.o
  GEN-SKEL [test_progs] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_progs-no_alu32] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
  ...
  CLNG-BPF [test_progs-cpuv4] access_map_in_map.bpf.o
  GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h

Fixes: a5d0c26a2784 ("selftests/bpf: Add a cpuv4 test runner for cpu=v4 testing")
Fixes: 89ad7420b25c ("selftests/bpf: Drop the need for LLVM's llc")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/Makefile | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 0b4bfbc0ef68..41acba62b8c4 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -428,23 +428,24 @@ $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
 # $1 - input .c file
 # $2 - output .o file
 # $3 - CFLAGS
+# $4 - binary name
 define CLANG_BPF_BUILD_RULE
-	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
+	$(call msg,CLNG-BPF,$4,$2)
 	$(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v3 -o $2
 endef
 # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
 define CLANG_NOALU32_BPF_BUILD_RULE
-	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
+	$(call msg,CLNG-BPF,$4,$2)
 	$(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v2 -o $2
 endef
 # Similar to CLANG_BPF_BUILD_RULE, but with cpu-v4
 define CLANG_CPUV4_BPF_BUILD_RULE
-	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
+	$(call msg,CLNG-BPF,$4,$2)
 	$(Q)$(CLANG) $3 -O2 --target=bpf -c $1 -mcpu=v4 -o $2
 endef
 # Build BPF object using GCC
 define GCC_BPF_BUILD_RULE
-	$(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
+	$(call msg,GCC-BPF,$4,$2)
 	$(Q)$(BPF_GCC) $3 -DBPF_NO_PRESERVE_ACCESS_INDEX -Wno-attributes -O2 -c $1 -o $2
 endef
 
@@ -537,7 +538,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:				\
 	$$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,			\
 					  $(TRUNNER_BPF_CFLAGS)         \
 					  $$($$<-CFLAGS)		\
-					  $$($$<-$2-CFLAGS))
+					  $$($$<-$2-CFLAGS),$(TRUNNER_BINARY))
 
 $(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
-- 
2.34.1

Re: [PATCH bpf-next v2] selftests/bpf: Fix wrong binary in Makefile log output

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

Dear patch submitter,

CI has tested the following submission:
Status:     SUCCESS
Name:       [bpf-next,v2] selftests/bpf: Fix wrong binary in Makefile log output
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872645&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10017878438

No further action is necessary on your part.


Please note: this email is coming from an unmonitored mailbox. If you have
questions or feedback, please reach out to the Meta Kernel CI team at
kernel-ci@meta.com.

Re: [PATCH bpf-next v2] selftests/bpf: Fix wrong binary in Makefile log output

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

Dear patch submitter,

CI has tested the following submission:
Status:     FAILURE
Name:       [bpf-next,v2] selftests/bpf: Fix wrong binary in Makefile log output
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872645&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10043109164

Failed jobs:
test_maps-s390x-gcc: https://github.com/kernel-patches/bpf/actions/runs/10043109164/job/27755765176


Please note: this email is coming from an unmonitored mailbox. If you have
questions or feedback, please reach out to the Meta Kernel CI team at
kernel-ci@meta.com.

Re: [PATCH bpf-next v2] selftests/bpf: Fix wrong binary in Makefile log output

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

Dear patch submitter,

CI has tested the following submission:
Status:     SUCCESS
Name:       [bpf-next,v2] selftests/bpf: Fix wrong binary in Makefile log output
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872645&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10047154037

No further action is necessary on your part.


Please note: this email is coming from an unmonitored mailbox. If you have
questions or feedback, please reach out to the Meta Kernel CI team at
kernel-ci@meta.com.

Re: [PATCH bpf-next v2] selftests/bpf: Fix wrong binary in Makefile log output

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]

Dear patch submitter,

CI has tested the following submission:
Status:     FAILURE
Name:       [bpf-next,v2] selftests/bpf: Fix wrong binary in Makefile log output
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872645&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10050695919

Failed jobs:
test_progs_no_alu32-aarch64-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050695919/job/27779231748
test_progs_no_alu32-s390x-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050695919/job/27779297532
test_progs_no_alu32-x86_64-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050695919/job/27779286074
test_progs_no_alu32-x86_64-llvm-17: https://github.com/kernel-patches/bpf/actions/runs/10050695919/job/27779294437
test_progs_no_alu32-x86_64-llvm-18: https://github.com/kernel-patches/bpf/actions/runs/10050695919/job/27779298637

First test_progs failure (test_progs_no_alu32-aarch64-gcc):
#134 libbpf_get_fd_by_id_opts
libbpf: prog 'check_access': BPF program load failed: Invalid argument
libbpf: prog 'check_access': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27
0: (b7) r0 = 0                        ; R0_w=0
1: (79) r2 = *(u64 *)(r1 +0)
func 'bpf_lsm_bpf_map' arg0 has btf_id 2072 type STRUCT 'bpf_map'
2: R1=ctx() R2_w=trusted_ptr_bpf_map()
; if (map != (struct bpf_map *)&data_input) @ test_libbpf_get_fd_by_id_opts.c:29
2: (18) r3 = 0xffff0000c3072800       ; R3_w=map_ptr(map=data_input,ks=4,vs=4)
4: (5d) if r2 != r3 goto pc+4         ; R2_w=trusted_ptr_bpf_map() R3_w=map_ptr(map=data_input,ks=4,vs=4)
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27
5: (79) r0 = *(u64 *)(r1 +8)          ; R0_w=scalar() R1=ctx()
; if (fmode & FMODE_WRITE) @ test_libbpf_get_fd_by_id_opts.c:32
6: (67) r0 <<= 62                     ; R0_w=scalar(smax=0x4000000000000000,umax=0xc000000000000000,smin32=0,smax32=umax32=0,var_off=(0x0; 0xc000000000000000))
7: (c7) r0 s>>= 63                    ; R0_w=scalar(smin=smin32=-1,smax=smax32=0)
;  @ test_libbpf_get_fd_by_id_opts.c:0
8: (57) r0 &= -13                     ; R0_w=scalar(smax=0x7ffffffffffffff3,umax=0xfffffffffffffff3,smax32=0x7ffffff3,umax32=0xfffffff3,var_off=(0x0; 0xfffffffffffffff3))
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27
9: (95) exit
At program exit the register R0 has smax=9223372036854775795 should have been in [-4095, 0]
processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
-- END PROG LOAD LOG --
libbpf: prog 'check_access': failed to load: -22
libbpf: failed to load object 'test_libbpf_get_fd_by_id_opts'
libbpf: failed to load BPF skeleton 'test_libbpf_get_fd_by_id_opts': -22
test_libbpf_get_fd_by_id_opts:FAIL:test_libbpf_get_fd_by_id_opts__open_and_load unexpected error: -22


Please note: this email is coming from an unmonitored mailbox. If you have
questions or feedback, please reach out to the Meta Kernel CI team at
kernel-ci@meta.com.

Re: [PATCH bpf-next v2] selftests/bpf: Fix wrong binary in Makefile log output

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

Dear patch submitter,

CI has tested the following submission:
Status:     SUCCESS
Name:       [bpf-next,v2] selftests/bpf: Fix wrong binary in Makefile log output
Patchwork:  https://patchwork.kernel.org/project/netdevbpf/list/?series=872645&state=*
Matrix:     https://github.com/kernel-patches/bpf/actions/runs/10051528751

No further action is necessary on your part.


Please note: this email is coming from an unmonitored mailbox. If you have
questions or feedback, please reach out to the Meta Kernel CI team at
kernel-ci@meta.com.

Re: [PATCH bpf-next v2] selftests/bpf: Fix wrong binary in Makefile log output

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Fri, 19 Jul 2024 22:25:35 -0700 you wrote:
> Make log output incorrectly shows 'test_maps' as the binary name for every
> 'CLNG-BPF' build step, apparently picking up the last value defined for the
> $(TRUNNER_BINARY) variable. Update the 'CLANG_BPF_BUILD_RULE' variants to
> fix this confusing output.
> 
> Current output:
>   CLNG-BPF [test_maps] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs] access_map_in_map.skel.h
>   ...
>   CLNG-BPF [test_maps] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs-no_alu32] access_map_in_map.skel.h
>   ...
>   CLNG-BPF [test_maps] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs-cpuv4] access_map_in_map.skel.h
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2] selftests/bpf: Fix wrong binary in Makefile log output
    https://git.kernel.org/bpf/bpf-next/c/efb1d64030ef

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Thu, 18 Jul 2024 22:57:43 +0000 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v4] selftests/bpf: use auto-dependencies for test objects
    https://git.kernel.org/bpf/bpf-next/c/a3cc56cd2c20

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Alexei Starovoitov]

On Fri, Jul 19, 2024 at 11:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to bpf/bpf-next.git (master)
> by Andrii Nakryiko <andrii@kernel.org>:
>
> On Thu, 18 Jul 2024 22:57:43 +0000 you wrote:
> > 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.
> >
> > [...]
>
> Here is the summary with links:
>   - [bpf-next,v4] selftests/bpf: use auto-dependencies for test objects
>     https://git.kernel.org/bpf/bpf-next/c/a3cc56cd2c20

Andrii, Ihor,

not sure what happened, but 'make clean' now takes forever.
Pls take a look.

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

[Eduard Zingerman]

On Mon, 2024-07-22 at 17:39 -0700, Alexei Starovoitov wrote:

[...]

> Andrii, Ihor,
> 
> not sure what happened, but 'make clean' now takes forever.
> Pls take a look.

It happens under certain conditions, here is a scenario that behaves
badly for me:
- two branches:
  - 'master' at cca09a371fa7
  - 'tmp' with [0] applied on top of master
- Steps to repro:

    # cd selftests directory
    $ git checkout tmp
    $ git clean -xfd .      # be careful
    $ make -j test_progs
    $ git checkout master
    $ make clean
    
After which output looks as follows:

  CLNG-BPF [test_maps] access_map_in_map.bpf.o
  GEN-SKEL [test_progs] access_map_in_map.skel.h
  CLNG-BPF [test_maps] arena_atomics.bpf.o
  GEN-SKEL [test_progs] arena_atomics.skel.h
  CLNG-BPF [test_maps] arena_htab_asm.bpf.o
  GEN-SKEL [test_progs] arena_htab_asm.skel.h
  CLNG-BPF [test_maps] arena_htab.bpf.o
  GEN-SKEL [test_progs] arena_htab.skel.h
  CLNG-BPF [test_maps] arena_list.bpf.o
  GEN-SKEL [test_progs] arena_list.skel.h
  CLNG-BPF [test_maps] async_stack_depth.bpf.o
  GEN-SKEL [test_progs] async_stack_depth.skel.h
  CLNG-BPF [test_maps] atomic_bounds.bpf.o
  GEN-SKEL [test_progs] atomic_bounds.skel.h
  CLNG-BPF [test_maps] bad_struct_ops2.bpf.o
  GEN-SKEL [test_progs] bad_struct_ops2.skel.h
  CLNG-BPF [test_maps] bad_struct_ops.bpf.o
  GEN-SKEL [test_progs] bad_struct_ops.skel.h
  CLNG-BPF [test_maps] bench_local_storage_create.bpf.o
  GEN-SKEL [test_progs] bench_local_storage_create.skel.h
  CLNG-BPF [test_maps] bind4_prog.bpf.o
  GEN-SKEL [test_progs] bind4_prog.skel.h
  ...

[0] https://lore.kernel.org/bpf/20240719110059.797546-1-xukuohai@huaweicloud.com/

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

[Eduard Zingerman]

On Mon, 2024-07-22 at 17:57 -0700, Eduard Zingerman wrote:
> On Mon, 2024-07-22 at 17:39 -0700, Alexei Starovoitov wrote:
> 
> [...]
> 
> > Andrii, Ihor,
> > 
> > not sure what happened, but 'make clean' now takes forever.
> > Pls take a look.
> 
> It happens under certain conditions, here is a scenario that behaves
> badly for me:
> - two branches:
>   - 'master' at cca09a371fa7
>   - 'tmp' with [0] applied on top of master
> - Steps to repro:
> 
>     # cd selftests directory
>     $ git checkout tmp
>     $ git clean -xfd .      # be careful
>     $ make -j test_progs
>     $ git checkout master
>     $ make clean
>     
> After which output looks as follows:
> 
>   CLNG-BPF [test_maps] access_map_in_map.bpf.o
>   GEN-SKEL [test_progs] access_map_in_map.skel.h
>   CLNG-BPF [test_maps] arena_atomics.bpf.o
>   GEN-SKEL [test_progs] arena_atomics.skel.h
>   ...
> 
> [0] https://lore.kernel.org/bpf/20240719110059.797546-1-xukuohai@huaweicloud.com/

Here is a funny part.
The switch from 'tmp' to 'master' touches a number of prog_tests/*.c files.
The output of 'make --debug=basic clean' is:

    Reading makefiles...
    Updating makefiles....
      CLNG-BPF [test_maps] access_map_in_map.bpf.o
      GEN-SKEL [test_progs] access_map_in_map.skel.h

There are .test.d files after test_progs build for tmp.
Because of the include directive:

    include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))

and a dependency:

    $(TRUNNER_TEST_OBJS:.o=.d): $(TRUNNER_OUTPUT)/%.test.d:			\
			    $(TRUNNER_TESTS_DIR)/%.c			\
			    ...

make goes ahead and detects that these .test.d files are now outdated.
So, before executing 'clean' recipe it executes recipes to update
.test.d files.

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

[Ihor Solodrai]

On Monday, July 22nd, 2024 at 5:57 PM, Eduard Zingerman <eddyz87@gmail.com> wrote:

> On Mon, 2024-07-22 at 17:39 -0700, Alexei Starovoitov wrote:
> 
> [...]
> 
> > Andrii, Ihor,
> > 
> > not sure what happened, but 'make clean' now takes forever.
> > Pls take a look.
> 
> 
> It happens under certain conditions, here is a scenario that behaves
> badly for me:
> [...]

This is an oversight in the auto-dependency patch...

Make automagically rebuilds dependencies of included makefiles if they have changed.

So, for example, if you do:

    $ make -j
    $ touch progs/verifier_and.c
    $ make clean

You'll get:

  CLNG-BPF [test_maps] verifier_and.bpf.o
  GEN-SKEL [test_progs] verifier_and.skel.h
  CLNG-BPF [test_maps] verifier_and.bpf.o
  GEN-SKEL [test_progs-cpuv4] verifier_and.skel.h
  CLNG-BPF [test_maps] verifier_and.bpf.o
  GEN-SKEL [test_progs-no_alu32] verifier_and.skel.h
  CLEAN    
  CLEAN   /opt/linux/tools/testing/selftests/bpf/bpf_testmod/Module.symvers
  CLEAN   eBPF_helpers-manpage
  CLEAN   eBPF_syscall-manpage

That is, dependencies of verifier_and.test.d are rebuilt, which would
be appropriate for other targets like test_progs.

I found a suggested workaround in makefile docs [1]:

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 05b234248..74f829952 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -608,7 +608,9 @@ $(TRUNNER_TEST_OBJS:.o=.d): $(TRUNNER_OUTPUT)/%.test.d:                     \
                            $(TRUNNER_BPF_SKELS_LINKED)                 \
                            $$(BPFOBJ) | $(TRUNNER_OUTPUT)
 
+ifeq ($(filter clean docs-clean,$(MAKECMDGOALS)),)
 include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
+endif
 
 $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:                          \
                       %.c                                              \

Basically, we can list the targets for which %.d files should be
ignored.

I suppose "clean" and "docs-clean" are the only relevant clean targets?

[1] https://www.gnu.org/software/make/manual/make.html#Goals

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

[Ihor Solodrai]

Andrii,

I looked over the v4 of the patch, and apparently I messed it up by
losing the v1 -> v2 change. So the issue with dump order of %.test.d
relative to %.test.o files is present on the master branch right now.

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 74f829952..4bcb1d1ce 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -596,7 +596,7 @@ 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_OUTPUT)/%.test.d
+                     | $(TRUNNER_OUTPUT)/%.test.d
        $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
        $(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -MMD -MT $$@ -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)

I can send this fix together with the condition for the clean targets
(so [1] can be discarded); or I can submit a separate change. Let me
know what you'd prefer.


I also had a discussion with Eduard off-list, he suggested trying to
remove explicit %.test.d targets altogether like this:

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 05b234248b38..f01dc1cc8af8 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -596,18 +596,12 @@ 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_OUTPUT)/%.test.d
> +		      | $(TRUNNER_BPF_SKELS)				\
> +		      	$(TRUNNER_BPF_LSKELS)				\
> +		      	$(TRUNNER_BPF_SKELS_LINKED)
>  	$$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
>  	$(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:				\
> -- 
> 2.45.2

This works almost as we want it, except for a situation when any
%.test.d gets deleted (say, due to local branch switch). In such case,
if one forgets to run `make clean`, there is no dependency of the
%.test.o on skels, and so they won't be properly updated.

After some discussion, me and Ed concluded that we shouldn't expect
people to remember to do clean in particular situations, especially if
consequences are not obvious. So the state after the suggested fixes
would be good enough.

[1] http://lore.kernel.org/K69Y8OKMLXBWR0dtOfsC4J46-HxeQfvqoFx1CysCm7u19HRx4MB6yAKOFkM6X-KAx2EFuCcCh_9vYWpsgQXnAer8oQ8PMeDEuiRMYECuGH4=@pm.me

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

[Andrii Nakryiko]

On Tue, Jul 23, 2024 at 12:25 PM Ihor Solodrai <ihor.solodrai@pm.me> wrote:
>
> Andrii,
>
> I looked over the v4 of the patch, and apparently I messed it up by
> losing the v1 -> v2 change. So the issue with dump order of %.test.d
> relative to %.test.o files is present on the master branch right now.
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 74f829952..4bcb1d1ce 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -596,7 +596,7 @@ 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_OUTPUT)/%.test.d
> +                     | $(TRUNNER_OUTPUT)/%.test.d
>         $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
>         $(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -MMD -MT $$@ -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
>
> I can send this fix together with the condition for the clean targets
> (so [1] can be discarded); or I can submit a separate change. Let me
> know what you'd prefer.

Send it separately, if that fix is good, I'll just apply it as is?

>
>
> I also had a discussion with Eduard off-list, he suggested trying to
> remove explicit %.test.d targets altogether like this:
>
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 05b234248b38..f01dc1cc8af8 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -596,18 +596,12 @@ 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_OUTPUT)/%.test.d
> > +                   | $(TRUNNER_BPF_SKELS)                            \
> > +                     $(TRUNNER_BPF_LSKELS)                           \
> > +                     $(TRUNNER_BPF_SKELS_LINKED)

shouldn't we also have a dependency on libbpf.a here as well, then? So
that all the auto-generated headers are autogenerated.

> >       $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
> >       $(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:                                \
> > --
> > 2.45.2
>
> This works almost as we want it, except for a situation when any
> %.test.d gets deleted (say, due to local branch switch). In such case,
> if one forgets to run `make clean`, there is no dependency of the
> %.test.o on skels, and so they won't be properly updated.
>
> After some discussion, me and Ed concluded that we shouldn't expect
> people to remember to do clean in particular situations, especially if
> consequences are not obvious. So the state after the suggested fixes
> would be good enough.
>

ok

> [1] http://lore.kernel.org/K69Y8OKMLXBWR0dtOfsC4J46-HxeQfvqoFx1CysCm7u19HRx4MB6yAKOFkM6X-KAx2EFuCcCh_9vYWpsgQXnAer8oQ8PMeDEuiRMYECuGH4=@pm.me
>
>

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

[Ihor Solodrai]

On Tuesday, July 23rd, 2024 at 1:02 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

[...]
> > 
> > I can send this fix together with the condition for the clean targets
> > (so [1] can be discarded); or I can submit a separate change. Let me
> > know what you'd prefer.
> 
> 
> Send it separately, if that fix is good, I'll just apply it as is?

Ok. Yes, you can apply the "if clean" patch as is.

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

[Björn Töpel]

patchwork-bot+netdevbpf@kernel.org writes:

> Hello:
>
> This patch was applied to bpf/bpf-next.git (master)
> by Andrii Nakryiko <andrii@kernel.org>:
>
> On Thu, 18 Jul 2024 22:57:43 +0000 you wrote:
>> 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.
>> 
>> [...]
>
> Here is the summary with links:
>   - [bpf-next,v4] selftests/bpf: use auto-dependencies for test objects
>     https://git.kernel.org/bpf/bpf-next/c/a3cc56cd2c20

I'm getting some build regressions for out-of-tree selftest build with
this patch on bpf-next/master. I'm building the selftests from the
selftest root, typically:

  make O=/output/foo SKIP_TARGETS="" \
    KSFT_INSTALL_PATH=/output/foo/kselftest \
    -C tools/testing/selftests install

and then package the whole output kselftest directory, and use that to
populate the DUT.

Reverting this patch, resolves the issues.

Two issues:

  1. The install target fails, resulting in many test scripts not copied
     to the install directory (e.g. test_kmod.sh).
  2. Building "all" target fails the second time.

To reproduce, do the following:

  Pre-requisite
    Build the kernel for yourfavorite arch -- my is RISC-V at moment ;-)

    make O=/output/foo defconfig
    make O=/output/foo kselftest-merge
    make O=/output/foo
    make O=/output/foo headers
  
  1. Install fail
    make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
      KSFT_INSTALL_PATH=/output/foo/kselftest \
      -C tools/testing/selftests install
  
  2. Build "all" fails the second time
      make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
      KSFT_INSTALL_PATH=/output/foo/kselftest \
      -C tools/testing/selftests
  
      make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
      KSFT_INSTALL_PATH=/output/foo/kselftest \
      -C tools/testing/selftests


Any ideas on a workaround? 

(And not related to this patch; It's annoying that "bpf" is the default
SKIP_TARGETS in kselftest. A bit meh 2024. ;-))


Björn

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

[Ihor Solodrai]

On Friday, September 13th, 2024 at 7:51 AM, Björn Töpel <bjorn@kernel.org> wrote:

> I'm getting some build regressions for out-of-tree selftest build with
> this patch on bpf-next/master. I'm building the selftests from the
> selftest root, typically:
> 
> make O=/output/foo SKIP_TARGETS="" \
> KSFT_INSTALL_PATH=/output/foo/kselftest \
> -C tools/testing/selftests install
> 
> and then package the whole output kselftest directory, and use that to
> populate the DUT.
> 
> Reverting this patch, resolves the issues.
> 
> Two issues:
> 
> 1. The install target fails, resulting in many test scripts not copied
> to the install directory (e.g. test_kmod.sh).
> 2. Building "all" target fails the second time.
> 
> To reproduce, do the following:
> 
> Pre-requisite
> Build the kernel for yourfavorite arch -- my is RISC-V at moment ;-)
> 
> make O=/output/foo defconfig
> make O=/output/foo kselftest-merge
> make O=/output/foo
> make O=/output/foo headers
> 
> 1. Install fail
> make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
> KSFT_INSTALL_PATH=/output/foo/kselftest \
> -C tools/testing/selftests install
> 
> 2. Build "all" fails the second time
> make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
> KSFT_INSTALL_PATH=/output/foo/kselftest \
> -C tools/testing/selftests
> 
> make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
> KSFT_INSTALL_PATH=/output/foo/kselftest \
> -C tools/testing/selftests
> 
> 
> Any ideas on a workaround?

Hi Björn.

I was able to reproduce the problem on bpf-next/master.

I found that in commit
https://git.kernel.org/bpf/bpf-next/c/f957c230e173 [1] the file
tools/testing/selftests/bpf/test_skb_cgroup_id.sh was deleted, but not
removed from the TEST_PROGS list in tools/testing/selftests/bpf/Makefile

Because of that rsync command (invoked by install target) fails:

    rsync: [sender] link_stat "/opt/linux/tools/testing/selftests/bpf/test_skb_cgroup_id.sh" failed: No such file or directory (2)
    rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1333) [sender=3.2.3]
    make[1]: *** [../lib.mk:175: install] Error 23
    make[1]: Leaving directory '/opt/linux/tools/testing/selftests/bpf'
    make: *** [Makefile:259: install] Error 2
    make: Leaving directory '/opt/linux/tools/testing/selftests'


After I removed test_skb_cgroup_id.sh from TEST_PROGS list, the
install target completed successfully.

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f04af11df8eb..df75f1beb731 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -132,7 +132,6 @@ TEST_PROGS := test_kmod.sh \
        test_tunnel.sh \
        test_lwt_seg6local.sh \
        test_lirc_mode2.sh \
-       test_skb_cgroup_id.sh \
        test_flow_dissector.sh \
        test_xdp_vlan_mode_generic.sh \
        test_xdp_vlan_mode_native.sh \

Could you please check on your side if this helps? Maybe there are
other issues.

[1] https://lore.kernel.org/bpf/20240813-convert_cgroup_tests-v4-4-a33c03458cf6@bootlin.com/

> 
> (And not related to this patch; It's annoying that "bpf" is the default
> SKIP_TARGETS in kselftest. A bit meh 2024. ;-))
> 
> 
> Björn
> 

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

[Björn Töpel]

Ihor Solodrai <ihor.solodrai@pm.me> writes:

> On Friday, September 13th, 2024 at 7:51 AM, Björn Töpel <bjorn@kernel.org> wrote:
>
>> I'm getting some build regressions for out-of-tree selftest build with
>> this patch on bpf-next/master. I'm building the selftests from the
>> selftest root, typically:
>> 
>> make O=/output/foo SKIP_TARGETS="" \
>> KSFT_INSTALL_PATH=/output/foo/kselftest \
>> -C tools/testing/selftests install
>> 
>> and then package the whole output kselftest directory, and use that to
>> populate the DUT.
>> 
>> Reverting this patch, resolves the issues.
>> 
>> Two issues:
>> 
>> 1. The install target fails, resulting in many test scripts not copied
>> to the install directory (e.g. test_kmod.sh).
>> 2. Building "all" target fails the second time.
>> 
>> To reproduce, do the following:
>> 
>> Pre-requisite
>> Build the kernel for yourfavorite arch -- my is RISC-V at moment ;-)
>> 
>> make O=/output/foo defconfig
>> make O=/output/foo kselftest-merge
>> make O=/output/foo
>> make O=/output/foo headers
>> 
>> 1. Install fail
>> make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
>> KSFT_INSTALL_PATH=/output/foo/kselftest \
>> -C tools/testing/selftests install
>> 
>> 2. Build "all" fails the second time
>> make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
>> KSFT_INSTALL_PATH=/output/foo/kselftest \
>> -C tools/testing/selftests
>> 
>> make O=/output/foo TARGETS=bpf SKIP_TARGETS="" \
>> KSFT_INSTALL_PATH=/output/foo/kselftest \
>> -C tools/testing/selftests
>> 
>> 
>> Any ideas on a workaround?
>
> Hi Björn.
>
> I was able to reproduce the problem on bpf-next/master.
>
> I found that in commit
> https://git.kernel.org/bpf/bpf-next/c/f957c230e173 [1] the file
> tools/testing/selftests/bpf/test_skb_cgroup_id.sh was deleted, but not
> removed from the TEST_PROGS list in tools/testing/selftests/bpf/Makefile
>
> Because of that rsync command (invoked by install target) fails:
>
>     rsync: [sender] link_stat "/opt/linux/tools/testing/selftests/bpf/test_skb_cgroup_id.sh" failed: No such file or directory (2)
>     rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1333) [sender=3.2.3]
>     make[1]: *** [../lib.mk:175: install] Error 23
>     make[1]: Leaving directory '/opt/linux/tools/testing/selftests/bpf'
>     make: *** [Makefile:259: install] Error 2
>     make: Leaving directory '/opt/linux/tools/testing/selftests'
>
>
> After I removed test_skb_cgroup_id.sh from TEST_PROGS list, the
> install target completed successfully.
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f04af11df8eb..df75f1beb731 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -132,7 +132,6 @@ TEST_PROGS := test_kmod.sh \
>         test_tunnel.sh \
>         test_lwt_seg6local.sh \
>         test_lirc_mode2.sh \
> -       test_skb_cgroup_id.sh \
>         test_flow_dissector.sh \
>         test_xdp_vlan_mode_generic.sh \
>         test_xdp_vlan_mode_native.sh \
>
> Could you please check on your side if this helps? Maybe there are
> other issues.

I don't even get that far in the "install" target. When I revert the
patch, I get to the issue you describe above (trying to install
non-existing file).

Here's an excerpt from a failed "install":

  |   BINARY   test_progs
  |   BINARY   test_progs-no_alu32
  |   BINARY   test_progs-cpuv4
  | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'

At this point the "all" target is complete; All good, and the "install"
is started. 

  | mkdir -p /home/bjorn/output/foo/kselftest/kselftest
  | install -m 744 kselftest/module.sh /home/bjorn/output/foo/kselftest/kselftest/
  | install -m 744 kselftest/runner.sh /home/bjorn/output/foo/kselftest/kselftest/
  | install -m 744 kselftest/prefix.pl /home/bjorn/output/foo/kselftest/kselftest/
  | install -m 744 kselftest/ktap_helpers.sh /home/bjorn/output/foo/kselftest/kselftest/
  | install -m 744 kselftest/ksft.py /home/bjorn/output/foo/kselftest/kselftest/
  | install -m 744 run_kselftest.sh /home/bjorn/output/foo/kselftest/
  | rm -f /home/bjorn/output/foo/kselftest/kselftest-list.txt

This is from the top-level "tools/testing/selftests/Makefile", and we
enter the BPF directory for "install".

  | make[1]: Entering directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
  | 
  | Auto-detecting system features:
  | ...                                    llvm: [ OFF ]
  | 
  |   LINK-BPF [test_progs] test_static_linked.bpf.o
  |   LINK-BPF [test_progs] linked_funcs.bpf.o
  |   LINK-BPF [test_progs] linked_vars.bpf.o
  |   LINK-BPF [test_progs] linked_maps.bpf.o
  |   LINK-BPF [test_progs] test_subskeleton.bpf.o
  |   LINK-BPF [test_progs] test_subskeleton_lib.bpf.o
  | ...
  |   EXT-COPY [test_maps]
  | make[1]: *** No rule to make target 'atomics.lskel.h', needed by '/home/bjorn/output/foo/kselftest/bpf/atomics.test.o'.  Stop.
  | make[1]: *** Waiting for unfinished jobs....
  | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
  | make: *** [Makefile:259: install] Error 2
  | make: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests'

...and for some reason the auto-dependencies decides to re-build, and
fails.

So, unfortunately it's something else related to your patch.

A reproducer:
  | $ docker run --rm -it --volume ${PWD}:/build/my-linux ghcr.io/linux-riscv/pw-builder bash -l
  | # cd /build/my-linux
  | # cat > ./build.sh <<EOF
  | #!/bin/bash
  | set -euo pipefail
  | 
  | rm -rf /output/foo
  | mkdir -p /output/foo
  | 
  | export PATH=\$(echo \$PATH | tr : "\n"| grep -v ^/opt | tr "\n" :)
  | 
  | make -j \$((\$(nproc)-1)) O=/output/foo defconfig
  | make -j \$((\$(nproc)-1)) O=/output/foo kselftest-merge
  | make -j \$((\$(nproc)-1)) O=/output/foo
  | make -j \$((\$(nproc)-1)) O=/output/foo headers
  | make -j \$((\$(nproc)-1)) O=/output/foo TARGETS=bpf SKIP_TARGETS="" KSFT_INSTALL_PATH=/output/foo/kselftest -C tools/testing/selftests install
  | EOF
  | 
  | # chmod +x ./build.sh
  | # ./build.sh


Björn

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

[Ihor Solodrai]

On Saturday, September 14th, 2024 at 3:54 AM, Björn Töpel <bjorn@kernel.org> wrote:

[...]

> > 
> > Could you please check on your side if this helps? Maybe there are
> > other issues.
> 
> 
> I don't even get that far in the "install" target. When I revert the
> patch, I get to the issue you describe above (trying to install
> non-existing file).
> 
> Here's an excerpt from a failed "install":
> 
> | BINARY test_progs
> | BINARY test_progs-no_alu32
> | BINARY test_progs-cpuv4
> | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
> 
> At this point the "all" target is complete; All good, and the "install"
> is started.
> 
> | mkdir -p /home/bjorn/output/foo/kselftest/kselftest
> | install -m 744 kselftest/module.sh /home/bjorn/output/foo/kselftest/kselftest/
> | install -m 744 kselftest/runner.sh /home/bjorn/output/foo/kselftest/kselftest/
> | install -m 744 kselftest/prefix.pl /home/bjorn/output/foo/kselftest/kselftest/
> | install -m 744 kselftest/ktap_helpers.sh /home/bjorn/output/foo/kselftest/kselftest/
> | install -m 744 kselftest/ksft.py /home/bjorn/output/foo/kselftest/kselftest/
> | install -m 744 run_kselftest.sh /home/bjorn/output/foo/kselftest/
> | rm -f /home/bjorn/output/foo/kselftest/kselftest-list.txt
> 
> This is from the top-level "tools/testing/selftests/Makefile", and we
> enter the BPF directory for "install".
> 
> | make[1]: Entering directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
> |
> | Auto-detecting system features:
> | ... llvm: [ OFF ]
> |
> | LINK-BPF [test_progs] test_static_linked.bpf.o
> | LINK-BPF [test_progs] linked_funcs.bpf.o
> | LINK-BPF [test_progs] linked_vars.bpf.o
> | LINK-BPF [test_progs] linked_maps.bpf.o
> | LINK-BPF [test_progs] test_subskeleton.bpf.o
> | LINK-BPF [test_progs] test_subskeleton_lib.bpf.o
> | ...
> | EXT-COPY [test_maps]
> | make[1]: *** No rule to make target 'atomics.lskel.h', needed by '/home/bjorn/output/foo/kselftest/bpf/atomics.test.o'. Stop.
> | make[1]: *** Waiting for unfinished jobs....
> | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
> | make: *** [Makefile:259: install] Error 2
> | make: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests'
> 
> ...and for some reason the auto-dependencies decides to re-build, and
> fails.
> 
> So, unfortunately it's something else related to your patch.

Björn,

I think I figured out the cause of the issue, but some details and a
proper solution are unclear yet.

In generated %.test.d makefiles some dependencies (in particular
%.skel.h) are referred to by filename as opposed to full path. For
example:

    $ cat /output/foo/kselftest/bpf/cpuv4/atomics.test.d
    /output/foo/kselftest/bpf/cpuv4/atomics.test.o: \
     /opt/linux/tools/testing/selftests/bpf/prog_tests/atomics.c \
     [...]
     /opt/linux/tools/testing/selftests/bpf/trace_helpers.h \
     /opt/linux/tools/testing/selftests/bpf/testing_helpers.h atomics.lskel.h \  # <-- THIS
     /output/foo/kselftest/bpf/tools/include/bpf/skel_internal.h \
     /output/foo/kselftest/bpf/tools/include/bpf/bpf.h

This is of course a problem, because make needs to know how to build a
target with this exact name. And in the patch it was (partially)
solved by this piece:

+# 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

This links %.skel.h to /output/foo/kselftest/bpf/no_alu32/%.skel.h and alike.

Your build is breaking because there is no such rule for
%.lskel.h and %.subskel.h, which are trivial to add:

--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -628,6 +628,12 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_OUTPUT)/%: $$$$(%-deps) $(BPFTOOL) | $(TR
 $(notdir %.skel.h): $(TRUNNER_OUTPUT)/%.skel.h
        @true
 
+$(notdir %.lskel.h): $(TRUNNER_OUTPUT)/%.lskel.h
+       @true
+
+$(notdir %.subskel.h): $(TRUNNER_OUTPUT)/%.subskel.h
+       @true
+
 endif

With this change a command below completed for me in the environment
you shared:

     $ make -j \$((\$(nproc)-1)) O=/output/foo TARGETS=bpf SKIP_TARGETS="" KSFT_INSTALL_PATH=/output/foo/kselftest -C tools/testing/selftests install


What is a mystery to me is why this was not an issue for simple `make
-C tool/testing/selftests/bpf`. And also why in the environment I
tried yesterday I didn't get the failure you're seeing.

Do you happen to have a Dockerfile of ghcr.io/linux-riscv/pw-builder ?
If possible, I'd like to look at it and compare with my local
environment.

The other issue that I'll have to think about is the unnecessary
re-builds that you've noticed. I suspect this happens due to the same
reason: a generated dependency on X.skel.h can't find a file in
current directory (because it was put to /output/foo), and so rebuild
is triggered. I'll have to come up with a workaround.


Björn, please try a change suggested above and let me know if it helps.

I will investigate these problems more, and there will definitely be a
follow up patch.

Thank you for reporting.

> 
> A reproducer:
> | $ docker run --rm -it --volume ${PWD}:/build/my-linux ghcr.io/linux-riscv/pw-builder bash -l
> | # cd /build/my-linux
> | # cat > ./build.sh <<EOF
> 
> | #!/bin/bash
> | set -euo pipefail
> |
> | rm -rf /output/foo
> | mkdir -p /output/foo
> |
> | export PATH=\$(echo \$PATH | tr : "\n"| grep -v ^/opt | tr "\n" :)
> |
> | make -j \$((\$(nproc)-1)) O=/output/foo defconfig
> | make -j \$((\$(nproc)-1)) O=/output/foo kselftest-merge
> | make -j \$((\$(nproc)-1)) O=/output/foo
> | make -j \$((\$(nproc)-1)) O=/output/foo headers
> | make -j \$((\$(nproc)-1)) O=/output/foo TARGETS=bpf SKIP_TARGETS="" KSFT_INSTALL_PATH=/output/foo/kselftest -C tools/testing/selftests install
> | EOF
> |
> | # chmod +x ./build.sh
> | # ./build.sh

And thank you for a reproducer, very helpful.

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

[Björn Töpel]

Ihor Solodrai <ihor.solodrai@pm.me> writes:

> On Saturday, September 14th, 2024 at 3:54 AM, Björn Töpel <bjorn@kernel.org> wrote:
>
> [...]
>
>> > 
>> > Could you please check on your side if this helps? Maybe there are
>> > other issues.
>> 
>> 
>> I don't even get that far in the "install" target. When I revert the
>> patch, I get to the issue you describe above (trying to install
>> non-existing file).
>> 
>> Here's an excerpt from a failed "install":
>> 
>> | BINARY test_progs
>> | BINARY test_progs-no_alu32
>> | BINARY test_progs-cpuv4
>> | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
>> 
>> At this point the "all" target is complete; All good, and the "install"
>> is started.
>> 
>> | mkdir -p /home/bjorn/output/foo/kselftest/kselftest
>> | install -m 744 kselftest/module.sh /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/runner.sh /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/prefix.pl /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/ktap_helpers.sh /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 kselftest/ksft.py /home/bjorn/output/foo/kselftest/kselftest/
>> | install -m 744 run_kselftest.sh /home/bjorn/output/foo/kselftest/
>> | rm -f /home/bjorn/output/foo/kselftest/kselftest-list.txt
>> 
>> This is from the top-level "tools/testing/selftests/Makefile", and we
>> enter the BPF directory for "install".
>> 
>> | make[1]: Entering directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
>> |
>> | Auto-detecting system features:
>> | ... llvm: [ OFF ]
>> |
>> | LINK-BPF [test_progs] test_static_linked.bpf.o
>> | LINK-BPF [test_progs] linked_funcs.bpf.o
>> | LINK-BPF [test_progs] linked_vars.bpf.o
>> | LINK-BPF [test_progs] linked_maps.bpf.o
>> | LINK-BPF [test_progs] test_subskeleton.bpf.o
>> | LINK-BPF [test_progs] test_subskeleton_lib.bpf.o
>> | ...
>> | EXT-COPY [test_maps]
>> | make[1]: *** No rule to make target 'atomics.lskel.h', needed by '/home/bjorn/output/foo/kselftest/bpf/atomics.test.o'. Stop.
>> | make[1]: *** Waiting for unfinished jobs....
>> | make[1]: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests/bpf'
>> | make: *** [Makefile:259: install] Error 2
>> | make: Leaving directory '/home/bjorn/src/linux/linux/tools/testing/selftests'
>> 
>> ...and for some reason the auto-dependencies decides to re-build, and
>> fails.
>> 
>> So, unfortunately it's something else related to your patch.
>
> Björn,
>
> I think I figured out the cause of the issue, but some details and a
> proper solution are unclear yet.
>
> In generated %.test.d makefiles some dependencies (in particular
> %.skel.h) are referred to by filename as opposed to full path. For
> example:
>
>     $ cat /output/foo/kselftest/bpf/cpuv4/atomics.test.d
>     /output/foo/kselftest/bpf/cpuv4/atomics.test.o: \
>      /opt/linux/tools/testing/selftests/bpf/prog_tests/atomics.c \
>      [...]
>      /opt/linux/tools/testing/selftests/bpf/trace_helpers.h \
>      /opt/linux/tools/testing/selftests/bpf/testing_helpers.h atomics.lskel.h \  # <-- THIS
>      /output/foo/kselftest/bpf/tools/include/bpf/skel_internal.h \
>      /output/foo/kselftest/bpf/tools/include/bpf/bpf.h
>
> This is of course a problem, because make needs to know how to build a
> target with this exact name. And in the patch it was (partially)
> solved by this piece:
>
> +# 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
>
> This links %.skel.h to /output/foo/kselftest/bpf/no_alu32/%.skel.h and alike.
>
> Your build is breaking because there is no such rule for
> %.lskel.h and %.subskel.h, which are trivial to add:
>
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -628,6 +628,12 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_OUTPUT)/%: $$$$(%-deps) $(BPFTOOL) | $(TR
>  $(notdir %.skel.h): $(TRUNNER_OUTPUT)/%.skel.h
>         @true
>  
> +$(notdir %.lskel.h): $(TRUNNER_OUTPUT)/%.lskel.h
> +       @true
> +
> +$(notdir %.subskel.h): $(TRUNNER_OUTPUT)/%.subskel.h
> +       @true
> +
>  endif
>
> With this change a command below completed for me in the environment
> you shared:
>
>      $ make -j \$((\$(nproc)-1)) O=/output/foo TARGETS=bpf SKIP_TARGETS="" KSFT_INSTALL_PATH=/output/foo/kselftest -C tools/testing/selftests install

Thank you! FWIW, this solves my build issue, and by extension making it
possible for the RISC-V CI to test BPF again! ;-)

Tested-by: Björn Töpel <bjorn@kernel.org>

Would be awesome if you can spin a patch proper for the above. Even if
it's uncomplete (by what you mention below), it solves my immediate
issue.

> What is a mystery to me is why this was not an issue for simple `make
> -C tool/testing/selftests/bpf`. And also why in the environment I
> tried yesterday I didn't get the failure you're seeing.
>
> Do you happen to have a Dockerfile of ghcr.io/linux-riscv/pw-builder ?
> If possible, I'd like to look at it and compare with my local
> environment.

Sure, it's at: https://github.com/linux-riscv/docker

Note that it's not something special. Simply Ubuntu 24.04 (noble), with
Clang/LLVM nightly from apt.llvm.org. I can reproduce this on my laptop,
and non-Docker builder that are running 24.04,a and Debian Sid.

> The other issue that I'll have to think about is the unnecessary
> re-builds that you've noticed. I suspect this happens due to the same
> reason: a generated dependency on X.skel.h can't find a file in
> current directory (because it was put to /output/foo), and so rebuild
> is triggered. I'll have to come up with a workaround.
>
>
> Björn, please try a change suggested above and let me know if it helps.
>
> I will investigate these problems more, and there will definitely be a
> follow up patch.
>
> Thank you for reporting.

Thank you for the swift response, and workaround! Much appreciated!

Now, enjoy the rest of the Sunday!


Cheers,
Björn