| Message ID | gJIk-oNcUE6_fdrEXMp0YBBlGqfyKiO6fE8KfjPvOeM9sq1eCphOVjbBziDVRWqIZK1gZZzDhbeIEeX41WA34qTz82izpkgG-F6EFTfX4IY=@pm.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] selftests/bpf: use auto-dependencies for test objects | expand |
On 7/12/24 6:36 AM, Ihor Solodrai wrote: > Make use of -M compiler options when building .test.o objects to > generate .d files and avoid re-building all tests every time. > > Previously, if a single test bpf program under selftests/bpf/progs/*.c > has changed, make would rebuild all the *.bpf.o, *.skel.h and *.test.o > objects, which is a lot of unnecessary work. > > A typical dependency chain is: > progs/x.c -> x.bpf.o -> x.skel.h -> x.test.o -> trunner_binary > > However for many tests it's not a 1:1 mapping by name, and so far > %.test.o have been simply dependent on all %.skel.h files, and > %.skel.h files on all %.bpf.o objects. > > Avoid full rebuilds by instructing the compiler (via -MMD) to > produce *.d files with real dependencies, and appropriately including > them. Exploit make feature that rebuilds included makefiles if they > were changed by setting %.test.d as prerequisite for %.test.o files. > > A couple of examples of compilation time speedup (after the first > clean build): > > $ touch progs/verifier_and.c && time make -j8 > Before: real 0m16.651s > After: real 0m2.245s > $ touch progs/read_vsyscall.c && time make -j8 > Before: real 0m15.743s > After: real 0m1.575s > > A drawback of this change is that now there is an overhead due to make > processing lots of .d files, which potentially may slow down unrelated > targets. However a time to make all from scratch hasn't changed > significantly: > > $ make clean && time make -j8 > Before: real 1m31.148s > After: real 1m30.309s > > Suggested-by: Eduard Zingerman <eddyz87@gmail.com> > Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me> > > --- > v1 -> v2: Make %.test.d prerequisite order only Looks like BPF CI trips over this and various tests are failing : https://github.com/kernel-patches/bpf/actions/runs/9902529566/job/27356664649 [...] Tests exit status: 1 Notice: Success: 538/3821, Skipped: 62, Failed: 5 Error: #66 core_reloc Error: #66/4 core_reloc/flavors Error: #66/4 core_reloc/flavors run_core_reloc_tests:FAIL:btf_src_file unexpected error: -1 (errno 2) Error: #66/5 core_reloc/flavors__err_wrong_name Error: #66/5 core_reloc/flavors__err_wrong_name run_core_reloc_tests:FAIL:btf_src_file unexpected error: -1 (errno 2) Error: #66/6 core_reloc/nesting Error: #66/6 core_reloc/nesting run_core_reloc_tests:FAIL:btf_src_file unexpected error: -1 (errno 2) Error: #66/7 core_reloc/nesting___anon_embed Error: #66/8 core_reloc/nesting___struct_union_mixup Error: #66/9 core_reloc/nesting___extra_nesting [...]
On Friday, July 12th, 2024 at 8:26 AM, Daniel Borkmann <daniel@iogearbox.net> wrote: [...] > Looks like BPF CI trips over this and various tests are failing : > > https://github.com/kernel-patches/bpf/actions/runs/9902529566/job/27356664649 > > [...] > Tests exit status: 1 > Notice: Success: 538/3821, Skipped: 62, Failed: 5 > Error: #66 core_reloc > Error: #66/4 core_reloc/flavors > Error: #66/4 core_reloc/flavors > run_core_reloc_tests:FAIL:btf_src_file unexpected error: -1 (errno 2) > Error: #66/5 core_reloc/flavors__err_wrong_name > Error: #66/5 core_reloc/flavors__err_wrong_name > run_core_reloc_tests:FAIL:btf_src_file unexpected error: -1 (errno 2) > Error: #66/6 core_reloc/nesting > Error: #66/6 core_reloc/nesting > run_core_reloc_tests:FAIL:btf_src_file unexpected error: -1 (errno 2) > Error: #66/7 core_reloc/nesting___anon_embed > Error: #66/8 core_reloc/nesting___struct_union_mixup > Error: #66/9 core_reloc/nesting___extra_nesting > [...] I've made a mistake when I removed $(TRUNNER_BPF_OBJS) as a prerequisite for $(TRUNNER_TEST_OBJS:.o=.d) I assumed it is covered by: $(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT) Apparently there are .bpf.o files for which skels are not generated, yet they are used in tests. Fixed in v3.
On Fri, 2024-07-12 at 17:48 +0000, Ihor Solodrai wrote: [...] > I've made a mistake when I removed $(TRUNNER_BPF_OBJS) as a > prerequisite for $(TRUNNER_TEST_OBJS:.o=.d) > > I assumed it is covered by: > > $(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT) > > Apparently there are .bpf.o files for which skels are not generated, > yet they are used in tests. > > Fixed in v3. 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 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? 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? [1] https://make.mad-scientist.net/constructed-include-files/
On Fri, Jul 12, 2024 at 12:06 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Fri, 2024-07-12 at 17:48 +0000, Ihor Solodrai wrote: > > [...] > > > I've made a mistake when I removed $(TRUNNER_BPF_OBJS) as a > > prerequisite for $(TRUNNER_TEST_OBJS:.o=.d) > > > > I assumed it is covered by: > > > > $(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT) > > > > Apparently there are .bpf.o files for which skels are not generated, > > yet they are used in tests. > > > > Fixed in v3. > > 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 > > 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? > > 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) I was about to say that not all tests use BPF skeleton headers just yet, so we have to have a way to explicitly specify dependencies. I think a separate list should be good enough for now, and in parallel we should try to switch remaining tests to skeleton headers. Even if we don't want to convert tests themselves to using skeleton structs, we can convert them to use elf_bytes from skeleton headers instead of loading .bpf.o files from disk. That should eliminate the need for extra dependencies. > > What are pros and cons between these two approaches? > > [1] https://make.mad-scientist.net/constructed-include-files/ >
On Fri, 2024-07-12 at 12:20 -0700, Andrii Nakryiko wrote: [...] > > 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) > > I was about to say that not all tests use BPF skeleton headers just > yet, so we have to have a way to explicitly specify dependencies. I > think a separate list should be good enough for now, and in parallel > we should try to switch remaining tests to skeleton headers. Even if > we don't want to convert tests themselves to using skeleton structs, > we can convert them to use elf_bytes from skeleton headers instead of > loading .bpf.o files from disk. That should eliminate the need for > extra dependencies. For the scope of this patch-set, I'd say specifying dependencies in the Makefile should be ok. Or would you prefer migrating tests to use elf bytes? [...]
On Fri, Jul 12, 2024 at 12:47 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Fri, 2024-07-12 at 12:20 -0700, Andrii Nakryiko wrote: > > [...] > > > > 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) > > > > I was about to say that not all tests use BPF skeleton headers just > > yet, so we have to have a way to explicitly specify dependencies. I > > think a separate list should be good enough for now, and in parallel > > we should try to switch remaining tests to skeleton headers. Even if > > we don't want to convert tests themselves to using skeleton structs, > > we can convert them to use elf_bytes from skeleton headers instead of > > loading .bpf.o files from disk. That should eliminate the need for > > extra dependencies. > > For the scope of this patch-set, I'd say specifying dependencies > in the Makefile should be ok. > Or would you prefer migrating tests to use elf bytes? 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. > > [...]
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? 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@gmail.com> 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.
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? > > 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@gmail.com> 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
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
On Tuesday, July 16th, 2024 at 4:21 PM, Eduard Zingerman <eddyz87@gmail.com> wrote: [...] > 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 might be nit-picking, but just so it's clear this target makes each individual %.bpf.o dependent on corresponding progs/%.c, and also on all the headers (because of *.h). I don't think we can easily remove/replace the $(TRUNNER_BPF_OBJS) target, as it also defines a compilation recipe for .bpf.o files. > > I think that simply replacing this catch all by something like: > > $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_BPF_OBJS) > Using a catch-all dependency (each %.test.o depends on *all* .bpf.o) is the current state of the Makefile, without the auto-dependencies patch: $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \ $(TRUNNER_TESTS_DIR)/%.c \ $(TRUNNER_EXTRA_HDRS) \ $(TRUNNER_BPF_OBJS) \ # this line $(TRUNNER_BPF_SKELS) \ ... In the v3 of the patch this dependency simply moved, such that each %.test.d file depends on *all* %.bpf.o, so essentially nothing changed there. So what we've been discussing so far is whether it's worth spending effort on removing/replacing this dependency, and how. Eduard's point about simplification of test cases seems reasonable. However it looks to me that whatever way of handling direct .bpf.o dependencies we might agree on, it would lead to an independent patch series. And settling on catch-all solution (even "for now") means settling on v3 of the patch.
On Wed, 2024-07-17 at 00:36 +0000, Ihor Solodrai wrote: > On Tuesday, July 16th, 2024 at 4:21 PM, Eduard Zingerman <eddyz87@gmail.com> wrote: > > [...] > > > 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 might be nit-picking, but just so it's clear this target makes each > individual %.bpf.o dependent on corresponding progs/%.c, and also on > all the headers (because of *.h). On current master touch progs/verifier_and.c leads to complete rebuild of all bpf.o and test.o files. And here is one of the targets: $ make test_progs -p | grep tools/testing/selftests/bpf/sockmap_listen.test.o: | tr ' ' '\n' | head /home/eddy/work/bpf-next/tools/testing/selftests/bpf/sockmap_listen.test.o: prog_tests/sockmap_listen.c flow_dissector_load.h ip_check_defrag_frags.h /home/eddy/work/bpf-next/tools/testing/selftests/bpf/access_map_in_map.bpf.o /home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_atomics.bpf.o /home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_htab_asm.bpf.o /home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_htab.bpf.o /home/eddy/work/bpf-next/tools/testing/selftests/bpf/arena_list.bpf.o /home/eddy/work/bpf-next/tools/testing/selftests/bpf/async_stack_depth.bpf.o 17:57:16 bpf$ make test_progs -p | grep tools/testing/selftests/bpf/sockmap_listen.test.o: | tr ' ' '\n' | grep bpf\.o | wc -l 760 > I don't think we can easily remove/replace the $(TRUNNER_BPF_OBJS) > target, as it also defines a compilation recipe for .bpf.o files. I don't think removing $(TRUNNER_BPF_OBJS) was suggested in the thread. > > I think that simply replacing this catch all by something like: > > > > $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_BPF_OBJS) > > > > Using a catch-all dependency (each %.test.o depends on *all* .bpf.o) > is the current state of the Makefile, without the auto-dependencies > patch: > > $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \ > $(TRUNNER_TESTS_DIR)/%.c \ > $(TRUNNER_EXTRA_HDRS) \ > $(TRUNNER_BPF_OBJS) \ # this line > $(TRUNNER_BPF_SKELS) \ > ... Yes, sure, but this is bad, because it forces rebuild of all .test.o files. Having auto-dependencies allows to get rid of this. > In the v3 of the patch this dependency simply moved, such that each > %.test.d file depends on *all* %.bpf.o, so essentially nothing changed > there. > > So what we've been discussing so far is whether it's worth spending > effort on removing/replacing this dependency, and how. > > Eduard's point about simplification of test cases seems reasonable. > However it looks to me that whatever way of handling direct .bpf.o > dependencies we might agree on, it would lead to an independent patch > series. > > And settling on catch-all solution (even "for now") means settling on > v3 of the patch. I don't like .test.d dependency on all .bpf.o files (the v3 change) as it does not encode the dependency we have: test_progs depends on core_reloc.bpf.o at runtime.
On Tuesday, July 16th, 2024 at 5:57 PM, Eduard Zingerman <eddyz87@gmail.com> wrote: > I don't like .test.d dependency on all .bpf.o files (the v3 change) > as it does not encode the dependency we have: > test_progs depends on core_reloc.bpf.o at runtime. I talked to Eduard off-list, and in his view this is the most appropriate way to leave a catch-all dependency: $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_BPF_OBJS) As opposed to original: $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \ ... $(TRUNNER_BPF_OBJS) \ ... Or v3: $(TRUNNER_TEST_OBJS:.o=.d): $(TRUNNER_OUTPUT)/%.test.d: \ ... $(TRUNNER_BPF_OBJS) \ ...
On Tue, Jul 16, 2024 at 4:21 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > 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. 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. > 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? > > I looked through the grep results and sorted those into buckets: > - test changes don't simplify anything: [...] > 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 meh, if we can avoid this > - by adding "fake" #include <smth.skel.h>, or sure, "good enough" > - by adding "true" #include <smth.skel.h> and using elf_bytes, or better, because we actually have compile-time check that those skeletons are used (and all necessary skeletons are used) and with minimal code and logic changes > - 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). > > 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? 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) > > --- > > 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 \ > ... 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) > > 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
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 [...]
On Wed, Jul 17, 2024 at 4:24 PM Eduard Zingerman <eddyz87@gmail.com> 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. :) > > [...]
On Thursday, July 18th, 2024 at 8:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: [...] > > > > - 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 agree. I'll submit v4 with this change. > > [...] > > > > > 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. This seems worthwhile to look into, although I think it's a task independent of this patch. > > [...] > > > > > 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. I tried a small experiment, and specifying particular lib/bpf headers didn't help because of vmlinux.h I grepped the list of headers with: $ grep -rh 'include <bpf/' progs | sort -u #include <bpf/bpf_core_read.h> #include <bpf/bpf_endian.h> #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> #include <bpf/usdt.bpf.h> Then, changed $(TRUNNER_BPF_OBJS) dependencies like this: diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 66478446af9d..6fb03bb9b33a 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -480,6 +480,13 @@ xdp_features.skel.h-deps := xdp_features.bpf.o LINKED_BPF_OBJS := $(foreach skel,$(LINKED_SKELS),$($(skel)-deps)) LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(LINKED_BPF_OBJS)) +HEADERS_FOR_BPF_OBJS := bpf_core_read.h \ + bpf_endian.h \ + bpf_helpers.h \ + bpf_tracing.h \ + usdt.bpf.h +HEADERS_FOR_BPF_OBJS := $(addprefix $(BPFDIR)/,$(HEADERS_FOR_BPF_OBJS)) + # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on # $eval()) and pass control to DEFINE_TEST_RUNNER_RULES. # Parameters: @@ -530,14 +537,15 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o: \ $(TRUNNER_BPF_PROGS_DIR)/%.c \ $(TRUNNER_BPF_PROGS_DIR)/*.h \ $$(INCLUDE_DIR)/vmlinux.h \ - $(wildcard $(BPFDIR)/bpf_*.h) \ - $(wildcard $(BPFDIR)/*.bpf.h) \ + $(HEADERS_FOR_BPF_OBJS) \ | $(TRUNNER_OUTPUT) $$(BPFOBJ) $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@, \ $(TRUNNER_BPF_CFLAGS) \ $$($$<-CFLAGS) \ $$($$<-$2-CFLAGS)) This didn't help because $ touch ~/work/kernel-clean/tools/lib/bpf/bpf.h triggers rebuild of vmlinux.h, which depends on $(BPFTOOL), and bpftool depends on $(HOST_BPFOBJ) or $(BPFOBJ), and they in turn depend on all files in lib/bpf. And there is a direct dependency of $(TRUNNER_BPF_OBJS) on vmlinux.h, which looks like a real dependency to me, but maybe I don't know something.
On Thu, Jul 18, 2024 at 3:42 PM Ihor Solodrai <ihor.solodrai@pm.me> wrote: > > On Thursday, July 18th, 2024 at 8:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > [...] > > > > > > - 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 agree. I'll submit v4 with this change. > > > > > [...] > > > > > > > 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. > > This seems worthwhile to look into, although I think it's a task > independent of this patch. > > > > > [...] > > > > > > > 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. > > I tried a small experiment, and specifying particular lib/bpf headers > didn't help because of vmlinux.h > > I grepped the list of headers with: > > $ grep -rh 'include <bpf/' progs | sort -u > > #include <bpf/bpf_core_read.h> > #include <bpf/bpf_endian.h> > #include <bpf/bpf_helpers.h> > #include <bpf/bpf_tracing.h> > #include <bpf/usdt.bpf.h> > > Then, changed $(TRUNNER_BPF_OBJS) dependencies like this: > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index 66478446af9d..6fb03bb9b33a 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -480,6 +480,13 @@ xdp_features.skel.h-deps := xdp_features.bpf.o > LINKED_BPF_OBJS := $(foreach skel,$(LINKED_SKELS),$($(skel)-deps)) > LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(LINKED_BPF_OBJS)) > > +HEADERS_FOR_BPF_OBJS := bpf_core_read.h \ > + bpf_endian.h \ > + bpf_helpers.h \ > + bpf_tracing.h \ > + usdt.bpf.h > +HEADERS_FOR_BPF_OBJS := $(addprefix $(BPFDIR)/,$(HEADERS_FOR_BPF_OBJS)) > + > # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on > # $eval()) and pass control to DEFINE_TEST_RUNNER_RULES. > # Parameters: > @@ -530,14 +537,15 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o: \ > $(TRUNNER_BPF_PROGS_DIR)/%.c \ > $(TRUNNER_BPF_PROGS_DIR)/*.h \ > $$(INCLUDE_DIR)/vmlinux.h \ > - $(wildcard $(BPFDIR)/bpf_*.h) \ > - $(wildcard $(BPFDIR)/*.bpf.h) \ > + $(HEADERS_FOR_BPF_OBJS) \ I'd leave *.bpf.h as is, it's meant to be BPF-only header (going forward, at least) > | $(TRUNNER_OUTPUT) $$(BPFOBJ) > $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@, \ > $(TRUNNER_BPF_CFLAGS) \ > $$($$<-CFLAGS) \ > $$($$<-$2-CFLAGS)) > > This didn't help because > > $ touch ~/work/kernel-clean/tools/lib/bpf/bpf.h > > triggers rebuild of vmlinux.h, which depends on $(BPFTOOL), and > bpftool depends on $(HOST_BPFOBJ) or $(BPFOBJ), and they in turn > depend on all files in lib/bpf. > > And there is a direct dependency of $(TRUNNER_BPF_OBJS) on vmlinux.h, > which looks like a real dependency to me, but maybe I don't know > something. > Yeah, we need to be smarter with vmlinux.h, I think. I think dependencies are set up correct, though pessimistic. Any libbpf change can cause bpftool change, which can cause different vmlinux.h generation. All that probably has to stay. But we can generate temporary vmlinux.h on the side, and compare it with pre-existing vmlinux.h. If contents didn't change (and we shouldn't have any timestamps in vmlinux.h or anything that just changes from run to run), we shouldn't touch the original vmlinux.h. This way will avoid .bpf.o regeneration. (vmlinux.h generation itself is fast, so I wouldn't bother trying to avoid it)
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index 5025401323af..4e4aae8aa7ec 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -31,6 +31,7 @@ test_tcp_check_syncookie_user test_sysctl xdping test_cpp +*.d *.subskel.h *.skel.h *.lskel.h diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index dd49c1d23a60..254d92a3b791 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -477,7 +477,8 @@ xsk_xdp_progs.skel.h-deps := xsk_xdp_progs.bpf.o xdp_hw_metadata.skel.h-deps := xdp_hw_metadata.bpf.o xdp_features.skel.h-deps := xdp_features.bpf.o -LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) +LINKED_BPF_OBJS := $(foreach skel,$(LINKED_SKELS),$($(skel)-deps)) +LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(LINKED_BPF_OBJS)) # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on # $eval()) and pass control to DEFINE_TEST_RUNNER_RULES. @@ -556,7 +557,11 @@ $(TRUNNER_BPF_LSKELS): %.lskel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT) $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.llinked3.o) name $$(notdir $$(<:.bpf.o=_lskel)) > $$@ $(Q)rm -f $$(<:.o=.llinked1.o) $$(<:.o=.llinked2.o) $$(<:.o=.llinked3.o) -$(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT) +$(LINKED_BPF_OBJS): %: $(TRUNNER_OUTPUT)/% + +# .SECONDEXPANSION here allows to correctly expand %-deps variables as prerequisites +.SECONDEXPANSION: +$(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_OUTPUT)/%: $$$$(%-deps) $(BPFTOOL) | $(TRUNNER_OUTPUT) $$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.bpf.o)) $(Q)$$(BPFTOOL) gen object $$(@:.skel.h=.linked1.o) $$(addprefix $(TRUNNER_OUTPUT)/,$$($$(@F)-deps)) $(Q)$$(BPFTOOL) gen object $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked1.o) @@ -566,6 +571,14 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT) $(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@ $(Q)$$(BPFTOOL) gen subskeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$(@:.skel.h=.subskel.h) $(Q)rm -f $$(@:.skel.h=.linked1.o) $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked3.o) + +# When the compiler generates a %.d file, only skel basenames (not +# full paths) are specified as prerequisites for corresponding %.o +# file. This target makes %.skel.h basename dependent on full paths, +# linking generated %.d dependency with actual %.skel.h files. +$(notdir %.skel.h): $(TRUNNER_OUTPUT)/%.skel.h + @true + endif # ensure we set up tests.h header generation rule just once @@ -583,14 +596,20 @@ endif # Note: we cd into output directory to ensure embedded BPF object is found $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \ $(TRUNNER_TESTS_DIR)/%.c \ - $(TRUNNER_EXTRA_HDRS) \ - $(TRUNNER_BPF_OBJS) \ - $(TRUNNER_BPF_SKELS) \ - $(TRUNNER_BPF_LSKELS) \ - $(TRUNNER_BPF_SKELS_LINKED) \ - $$(BPFOBJ) | $(TRUNNER_OUTPUT) + | $(TRUNNER_OUTPUT)/%.test.d $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@) - $(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F) + $(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -MMD -MT $$@ -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F) + +$(TRUNNER_TEST_OBJS:.o=.d): $(TRUNNER_OUTPUT)/%.test.d: \ + $(TRUNNER_TESTS_DIR)/%.c \ + $(TRUNNER_EXTRA_HDRS) \ + $(TRUNNER_BPF_SKELS) \ + $(TRUNNER_BPF_LSKELS) \ + $(TRUNNER_BPF_SKELS_LINKED) \ + $$(BPFOBJ) | $(TRUNNER_OUTPUT) + +include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d)) + $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o: \ %.c \ @@ -768,8 +787,8 @@ $(OUTPUT)/uprobe_multi: uprobe_multi.c EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \ prog_tests/tests.h map_tests/tests.h verifier/tests.h \ - feature bpftool \ - $(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h *.subskel.h \ + feature bpftool \ + $(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h \ no_alu32 cpuv4 bpf_gcc bpf_testmod.ko \ bpf_test_no_cfi.ko \ liburandom_read.so)
Make use of -M compiler options when building .test.o objects to generate .d files and avoid re-building all tests every time. Previously, if a single test bpf program under selftests/bpf/progs/*.c has changed, make would rebuild all the *.bpf.o, *.skel.h and *.test.o objects, which is a lot of unnecessary work. A typical dependency chain is: progs/x.c -> x.bpf.o -> x.skel.h -> x.test.o -> trunner_binary However for many tests it's not a 1:1 mapping by name, and so far %.test.o have been simply dependent on all %.skel.h files, and %.skel.h files on all %.bpf.o objects. Avoid full rebuilds by instructing the compiler (via -MMD) to produce *.d files with real dependencies, and appropriately including them. Exploit make feature that rebuilds included makefiles if they were changed by setting %.test.d as prerequisite for %.test.o files. A couple of examples of compilation time speedup (after the first clean build): $ touch progs/verifier_and.c && time make -j8 Before: real 0m16.651s After: real 0m2.245s $ touch progs/read_vsyscall.c && time make -j8 Before: real 0m15.743s After: real 0m1.575s A drawback of this change is that now there is an overhead due to make processing lots of .d files, which potentially may slow down unrelated targets. However a time to make all from scratch hasn't changed significantly: $ make clean && time make -j8 Before: real 1m31.148s After: real 1m30.309s Suggested-by: Eduard Zingerman <eddyz87@gmail.com> Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me> --- v1 -> v2: Make %.test.d prerequisite order only --- tools/testing/selftests/bpf/.gitignore | 1 + tools/testing/selftests/bpf/Makefile | 41 +++++++++++++++++++------- 2 files changed, 31 insertions(+), 11 deletions(-)