On Friday, July 12th, 2024 at 12:06 PM, Eduard Zingerman <eddyz87@xxxxxxxxx> 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?
I tried to figure out what tests reference or depend on .bpf.o files
directly, and there seems to be much more of those.
I added some prints to the makefile, and across all TRUNNERs 291
generated .bpf.o objects do not have a corresponding (by name) .skel.h
file. Part of them are blacklisted btf__% and alike.
A grep of ".bpf.o" over the code gives 186 references:
$ grep -r '\.bpf\.o' --include="*.[ch]" | wc -l
186 # number of references
$ grep -rl '\.bpf\.o' --include="*.[ch]" | wc -l
58 # number of files
For example, bpf_prog_test_load helper is sometimes used to load
.bpf.o files, which introduces a direct dependency, as far as I
understand.
Of course we are talking about a subset of these dependencies: we want
those cases where a relevant skel is not included, while .bpf.o is
required. But we'd have to review each individual test (at least from
the grep list) to filter the subset. Or am I wrong about this?
I thought maybe to simply remove the dependency on $(TRUNNER_BPF_OBJS)
and see what breaks, but I have doubts it's a good approach.
> And we fix this by adding a dependency:
>
> $(TRUNNER_TEST_OBJS:.o=.d): ... $(TRUNNER_BPF_OBJS)
>
> Which makes all *.test.d files depend on .bpf.o files.
> Thus, if progs/some.c file is changed and `make test_progs` is requested:
> - because *.test.d files are included into current makefile [1],
> make invokes targets for *.test.d files;
> - *.test.d targets depend on *.bpf.o, thus some.bpf.o is rebuilt
> (but only some.bpf.o, dependencies for other *.bpf.o are up to date);
> - case A, skel for some.c is not included anywhere (CI failure for v2):
> - nothing happens further, as *.test.d files are unchanged *.test.o
> files are not rebuilt and test_progs is up to date
> - case B, skel for some.c is included in prog_tests/other.c:
> - existing other.test.d lists some.skel.h as a dependency;
> - this dependency is not up to date, so other.test.o is rebuilt;
> - test_progs is rebuilt.
>
> Do I understand the above correctly?
Yes. This is my understanding as well.
>
> An alternative fix would be to specify additional dependencies for
> core_reloc.test.o (and others) directly, e.g.:
>
> core_reloc.test.o: test_core_reloc_module.bpf.o ...
>
> (with correct trunner prefix)
>
> What are pros and cons between these two approaches?
Well, this is a common issue of whether to "include everything" or to
write an explicit list of dependencies.
So far the tests depended on "everything", and the idea of this patch
was to reduce repetitive tests compilation time by leveraging
auto-generated explicit dependencies.
However in the case with dynamically loaded .bpf.o, if we split the
dependency of %.test.d on $(TRUNNER_BPF_OBJS), it's not clear to me
what the benefits of that would be.
I can't think of a situation when all BPF objs have to be rebuilt from
scratch because of this dependency. And it was this way without the
patch too.
The only benefit I can see is that dependencies will be clearly listed
in the makefile. It's a good thing of course, but is it worth the
effort?
On Friday, July 12th, 2024 at 12:52 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> I don't particularly care. If we don't do that, then we waste some
> effort to specify dependencies manually, just to remove them later. So
> it might be worth it to do a quick switch to <skel>__elf_bytes(),
>
> ending up with a better end state earlier. But I don't feel strongly
> about any of this, so it's up to you guys.
If there are plans to eventually migrate all tests to use skels, then
I agree with Andrii that figuring out dependencies would be a wasted
effort.
But then the same can be said about switching to <skel>__elf_bytes(),
right?
I don't mind going over the tests to clear out dependencies or modify
the tests in some way. I just want to be sure it'll be in line with
the project goals. Obviously, I am new to the codebase and don't know
much about anything here, so I'm relying on your input.
