On Wed, Jul 17, 2024 at 4:24 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
>
Fair enough, I'm fine with that.
> > > 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
+1
>
> [...]
>
> > > 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.
still an improvement and will get us close. Right now there is no
incentive to do anything about those few special cases (like btf__*)
because of all the other .bpf.o dependencies.
>
> [...]
>
> > 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
yeah, ideally they wouldn't cause bpf.o rebuilds... I think we should
tune .bpf.o to depend only on BPF-side headers (we'd need to hard-code
them, but they don't change often: usdt.bpf.h, bpf_tracing.h,
bpf_helpers.h, etc). I don't think we can get rid of BPF skeletons'
dependency on bpftool (which depends on *any* libbpf change), though,
so .skel.h will be regenerated due to any tiny libbpf change, but
that's still better, as bpf.o building is probably the slowest part.
So. Many. Opportunities. :)
>
> [...]
