On Sun, Jul 14, 2024 at 6:17 PM Ihor Solodrai <ihor.solodrai@xxxxx> wrote:
>
> 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?
We do seem to have quite a bit of such dependencies indeed, roughly:
$ rg '\.bpf\.o' | wc -l
233
>
> 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.
Well, what would happen is that now you can never rely on make to
build the right thing when you modify something in progs/*.c, and so
paranoidally one would have to do `make clean && make -j$(nproc)` to
make sure all relevant object files are rebuilt.
>
> > 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?
>
I think we'll end up having small amount of explicit dependencies, at
least for these cases (and any other similar ones):
$ rg '\.bpf\.o' | rg __btf_path
progs/verifier_bitfield_write.c:__btf_path("btf__core_reloc_bitfields.bpf.o")
progs/verifier_bitfield_write.c:__btf_path("btf__core_reloc_bitfields.bpf.o")
progs/verifier_bitfield_write.c:__btf_path("btf__core_reloc_bitfields.bpf.o")
progs/verifier_bitfield_write.c:__btf_path("btf__core_reloc_bitfields.bpf.o")
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.
>
> 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.
I don't think there are plans, but it's definitely a desirable
outcome. Just no one signed up to do this rather mundane part of work.
So if you don't mind helping with this, that would be very much
appreciated (at least by me).
>
> But then the same can be said about switching to <skel>__elf_bytes(),
> right?
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 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.
>
+1 from me to convert to elf_bytes(), but let's see what other maintainers think
