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

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-07-17 at 00:36 +0000, Ihor Solodrai wrote:
> On Tuesday, July 16th, 2024 at 4:21 PM, Eduard Zingerman <eddyz87@gmail.com> wrote:
> 
> [...]
> 
> > The catch-all clause in the current makefile looks as follows:
> > 
> > $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o: \
> > $(TRUNNER_BPF_PROGS_DIR)/%.c \
> > $(TRUNNER_BPF_PROGS_DIR)/*.h \
> > ...
> > 
> > This makes all .bpf.o files dependent on all BPF .c files.
> > .bpf.o files rebuild is the main time consumer, at-least for me.
> 
> I might be nit-picking, but just so it's clear this target makes each
> individual %.bpf.o dependent on corresponding progs/%.c, and also on
> all the headers (because of *.h).

On current master touch progs/verifier_and.c leads to complete rebuild
of all bpf.o and test.o files. And here is one of the targets:

$ make test_progs -p | grep tools/testing/selftests/bpf/sockmap_listen.test.o: | tr ' ' '\n' | head
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/sockmap_listen.test.o:
prog_tests/sockmap_listen.c
flow_dissector_load.h
ip_check_defrag_frags.h
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/access_map_in_map.bpf.o
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_atomics.bpf.o
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_htab_asm.bpf.o
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_htab.bpf.o
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_list.bpf.o
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/async_stack_depth.bpf.o
17:57:16 bpf$ make test_progs -p | grep tools/testing/selftests/bpf/sockmap_listen.test.o: | tr ' ' '\n' | grep bpf\.o | wc -l
760

> I don't think we can easily remove/replace the $(TRUNNER_BPF_OBJS)
> target, as it also defines a compilation recipe for .bpf.o files.

I don't think removing $(TRUNNER_BPF_OBJS) was suggested in the thread.

> > I think that simply replacing this catch all by something like:
> > 
> > $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_BPF_OBJS)
> > 
> 
> Using a catch-all dependency (each %.test.o depends on *all* .bpf.o)
> is the current state of the Makefile, without the auto-dependencies
> patch:
> 
> $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:			\
> 		      $(TRUNNER_TESTS_DIR)/%.c				\
> 		      $(TRUNNER_EXTRA_HDRS)				\
> 		      $(TRUNNER_BPF_OBJS)				\  # this line
> 		      $(TRUNNER_BPF_SKELS)				\
>    ...

Yes, sure, but this is bad, because it forces rebuild of all .test.o files.
Having auto-dependencies allows to get rid of this.

> In the v3 of the patch this dependency simply moved, such that each
> %.test.d file depends on *all* %.bpf.o, so essentially nothing changed
> there.
> 
> So what we've been discussing so far is whether it's worth spending
> effort on removing/replacing this dependency, and how.
> 
> Eduard's point about simplification of test cases seems reasonable.
> However it looks to me that whatever way of handling direct .bpf.o
> dependencies we might agree on, it would lead to an independent patch
> series.
> 
> And settling on catch-all solution (even "for now") means settling on
> v3 of the patch.

I don't like .test.d dependency on all .bpf.o files (the v3 change)
as it does not encode the dependency we have:
test_progs depends on core_reloc.bpf.o at runtime.