diff mbox series

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

Message ID VJihUTnvtwEgv_mOnpfy7EgD9D2MPNoHO-MlANeLIzLJPGhDeyOuGKIYyKgk0O6KPjfM-MuhtvPwZcngN8WFqbTnTRyCSMc2aMZ1ODm1T_g=@pm.me (mailing list archive)
State Accepted
Commit a3cc56cd2c206b9460371af9ce0be65b2367aa58
Delegated to: BPF
Headers show
Series [bpf-next,v4] selftests/bpf: use auto-dependencies for test objects | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 661 this patch: 661
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 11 maintainers not CCed: kpsingh@kernel.org shuah@kernel.org haoluo@google.com andrii@kernel.org john.fastabend@gmail.com jolsa@kernel.org linux-kselftest@vger.kernel.org yonghong.song@linux.dev martin.lau@linux.dev song@kernel.org sdf@fomichev.me
netdev/build_clang success Errors and warnings before: 663 this patch: 663
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 667 this patch: 667
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 88 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18

Commit Message

Ihor Solodrai July 18, 2024, 10:57 p.m. UTC 
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(-)

Comments

Andrii Nakryiko July 19, 2024, 6:18 p.m. UTC | #1 
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

[...]
patchwork-bot+netdevbpf@kernel.org July 19, 2024, 6:20 p.m. UTC | #2 
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!
Ihor Solodrai July 19, 2024, 7:03 p.m. UTC | #3 
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 :)
Tony Ambardar July 19, 2024, 9:54 p.m. UTC | #4 
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
> 
> [...]
Andrii Nakryiko July 19, 2024, 10:25 p.m. UTC | #5 
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
> >
> > [...]
Alexei Starovoitov July 23, 2024, 12:39 a.m. UTC | #6 
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.
Eduard Zingerman July 23, 2024, 12:57 a.m. UTC | #7 
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/
Eduard Zingerman July 23, 2024, 1:49 a.m. UTC | #8 
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.
Ihor Solodrai July 23, 2024, 1:50 a.m. UTC | #9 
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
Ihor Solodrai July 23, 2024, 7:25 p.m. UTC | #10 
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
Andrii Nakryiko July 23, 2024, 8:02 p.m. UTC | #11 
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
>
>
Ihor Solodrai July 23, 2024, 8:11 p.m. UTC | #12 
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.
Björn Töpel Sept. 13, 2024, 2:51 p.m. UTC | #13 
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
Ihor Solodrai Sept. 13, 2024, 10:33 p.m. UTC | #14 
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
>
Björn Töpel Sept. 14, 2024, 10:54 a.m. UTC | #15 
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
Ihor Solodrai Sept. 15, 2024, 4:47 a.m. UTC | #16 
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.
Björn Töpel Sept. 15, 2024, 3:41 p.m. UTC | #17 
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
diff mbox series

Patch

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)