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

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-07-17 at 09:41 -0700, Andrii Nakryiko wrote:

[...]

> > 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.
> 
> Hm... "simplify tests" isn't the goal of this change. The goal is to
> speed up the build process (while not breaking dependencies). So I
> don't see simplification of any kind as a requirement. I'd say we
> shouldn't complicate tests (too much) just for this, but some light
> changes seem fine to me.

My point is that we don't need to update *any* tests to get 99.9% of
the speed up. Thus, the tests update should have some additional net
benefit. And I don't see much gains after looking through the tests.

> > By test simplification I mean at-least removal of some
> > bpf_object__find_{map,program}_by_name() calls.
> 
> Some tests are generic and need (or at least are more natural)
> lookup-by-name kind of APIs. Sure we can completely rewrite tests, but
> why?

Sure, I meant the tests where the above APIs were used to find a
single program or map etc, there are a few such tests.

[...]

> > - by adding a catch-all clause in the makefile, e.g. making test
> >   runner depend on all .bpf.o files.
> 
> do we actually need to rebuild final binary if we are still just
> loading .bpf.o from disk? We are not embedding such .bpf.o (embedding
> is what skeleton headers are adding), so why rebuild .bpf.o?
> 
> Actually thinking about this again, I guess, if we don't want to add
> skel.h to track precise dependencies, we don't really need to do
> anything extra for those progs/*.c files that are not used through
> skeletons. We just need to make sure that they are rebuilt if they are
> changed. The rest will work as is because test runner binary will just
> load them from disk at the next run (and user space part doesn't have
> to be rebuilt, unless it itself changed).

Good point. This can be achieved by making $(OUTPUT)/$(TRUNNER_BINARY)
dependency on $(TRUNNER_BPF_OBJS) order-only, e.g. here is a modified
version of the v2: https://tinyurl.com/4wnhkt32

[...]

> > 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?
> 
> see above, just because there is no guarantee that we use all the
> dependencies and we didn't miss any. It's not a high risk, but it's
> also trivial to switch to elf_bytes.
> 
> another side benefit of completely switching to .skel.h is that we can
> stop copying all .bpf.o files into BPF CI, because test_progs will be
> self-contained (thought that's not 100% true due to btf__* and maybe a
> few files more, which is sad and a bit different problem)

Hm, this might make sense.
There are 410Mb of .bpf.o files generated currently.
On the other hand, as you note, one would still need a list of some
.bpf.o files, because there are at-least several tests that verify
operation on ELF files, not ELF bytes.

[...]

> keep in mind that we do want to rebuild .bpf.o if libbpf's BPF-side
> headers changed, so let's make sure that stays (or happens, if we
> don't do it already)

Commands below cause full rebuild (.test.o, .bpf.o) on v2 of this
patch-set:
$ touch tools/lib/bpf/bpf.h
$ touch tools/lib/bpf/libbpf.h

[...]