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

[Eduard Zingerman <eddyz87@gmail.com>]

On Sun, Jul 14, 2024 at 6:17 PM Ihor Solodrai <ihor.solodrai@pm.me> wrote:
> On Friday, July 12th, 2024 at 12:06 PM, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > So, bear with me for a moment please.
> > We have 3 test_progs/smth.c files that depend on a few .bpf.o files at runtime,
> > but do not include skel files for those .bpf.o, namely:
> > - core_reloc.c
> > - verifier_bitfield_write.c
> > - pinning.c
>
> Is this an exhaustive list, or did you mean it just as an example?

My bad, I looked only at the tests that failed on CI, there are indeed
more dependencies.

On Mon, 2024-07-15 at 10:44 -0700, Andrii Nakryiko wrote:
> But I think the right direction is to get rid of file-based loading of
> .bpf.o in favor of a) proper skeleton usage (lots of work to rewrite
> portions of file, so not very hopeful here) or b) a quick-fix-like
> equivalent and pretty straightforward <skel>___elf_bytes() replacement
> everywhere where we currently load the same bytes from file on the
> disk.

I don't really see a point in migrating tests to use skels or
elf_bytes if such migration does not simplify the test case itself.
By test simplification I mean at-least removal of some
bpf_object__find_{map,program}_by_name() calls.

I looked through the grep results and sorted those into buckets:
- test changes don't simplify anything:
  - bpf_obj_id.c
  - bpf_verif_scale.c
  - btf.c
  - btf_endian.c
  - connect_force_port.c
  - core_reloc.c
  - fexit_bpf2bpf.c
  - global_data.c
  - lwt_redirect.c
  - lwt_reroute.c
  - map_lock.c
  - pkt_access.c
  - pkt_md_access.c
  - queue_stack_map.c
  - resolve_btfids.c
  - sk_assign.c
  - skb_ctx.c
  - skb_helpers.c
  - task_fd_query_rawtp.c
  - task_fd_query_tp.c
  - tp_attach_query.c
  - trampoline_count.c
  - xdp_adjust_frags.c
  - xdp_adjust_tail.c
  - xdp_attach.c
  - xdp_info.c
  - xdp_perf.c
- skel usage would somewhat simplify the test:
  - get_stack_raw_tp.c
  - global_data_init.c
  - global_func_args.c
  - kfree_skb.c
  - l4lb_all.c
  - load_bytes_relative.c
  - pinning.c
  - probe_user.c
  - rdonly_maps.c
  - select_reuseport.c
  - stacktrace_map.c
  - stacktrace_map_raw_tp.c
  - tailcalls.c
  - test_overhead.c
  - xdp.c
- can be migrated to test_loader:
  - reference_tracking.c
  - tcp_estats.c
  
Given the large number of test cases that don't seem to benefit from
skel rework, I think we would need to handle direct dependencies
somehow, e.g.:
- by writing down these dependencies in the makefile, or
- by adding "fake" #include <smth.skel.h>, or
- by adding "true" #include <smth.skel.h> and using elf_bytes, or
- by adding a catch-all clause in the makefile, e.g. making test
  runner depend on all .bpf.o files.

On Mon, 2024-07-15 at 10:44 -0700, Andrii Nakryiko wrote:
> see above, elf_bytes is a quick and dirty way to get rid of file
> dependencies and turn them into .skel.h dependency without having to
> change existing tests significantly (which otherwise would be tons of
> work).

I assume that the goal here is to encode dependencies via skel.h files
inclusion. For bpf selftests presence of skel.h guarantees presence of
the freshly built object file. Why bother with elf_bytes rework if
just including the skel files would be sufficient?

  ---

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 think that simply replacing this catch all by something like:

    $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_BPF_OBJS)

on top of v2 of this patch-set is a good stop gap solution:
it is simple, explicit and brings most of the speedup.
People rebuilding test_progs would only pay for compilation of BPF
object files that had been changed.

---

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 557078f2cf74..11316ccb5556 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -628,13 +628,16 @@ 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)                          \
                             $(TRUNNER_BPFTOOL)                         \
                             | $(TRUNNER_BINARY)-extras
        $$(call msg,BINARY,,$$@)
-       $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
+       $(Q)$$(CC) $$(CFLAGS) $$(filter-out %.bpf.o, $$(filter %.a %.o,$$^)) $$(LDLIBS) -o $$@
        $(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
        $(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
                   $(OUTPUT)/$(if $2,$2/)bpftool