On Fri, Aug 30, 2024 at 2:23 PM Mykyta Yatsenko <mykyta.yatsenko5@xxxxxxxxx> wrote: > > On 30/08/2024 21:34, Andrii Nakryiko wrote: > > On Wed, Aug 28, 2024 at 3:02 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > >> On Wed, 2024-08-28 at 17:46 +0000, Ihor Solodrai wrote: > >>> %.bpf.o objects depend on vmlinux.h, which makes them transitively > >>> dependent on unnecessary libbpf headers. However vmlinux.h doesn't > >>> actually change as often. > >>> > >>> When generating vmlinux.h, compare it to a previous version and update > >>> it only if there are changes. > >>> > >>> Example of build time improvement (after first clean build): > >>> $ touch ../../../lib/bpf/bpf.h > >>> $ time make -j8 > >>> Before: real 1m37.592s > >>> After: real 0m27.310s > >>> > >>> Notice that %.bpf.o gen step is skipped if vmlinux.h hasn't changed. > >>> > >>> Link: https://lore.kernel.org/bpf/CAEf4BzY1z5cC7BKye8=A8aTVxpsCzD=p1jdTfKC7i0XVuYoHUQ@xxxxxxxxxxxxxx > >>> > >>> Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxx> > >>> --- > >> Unfortunately, I think that this is a half-measure. > >> E.g. the following command forces tests rebuild for me: > >> > >> touch ../../../../kernel/bpf/verifier.c; \ > >> make -j22 -C ../../../../; \ > >> time make test_progs > >> > >> To workaround this we need to enable reproducible_build option: > >> > >> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf > >> index b75f09f3f424..8cd648f3e32b 100644 > >> --- a/scripts/Makefile.btf > >> +++ b/scripts/Makefile.btf > >> @@ -19,7 +19,7 @@ pahole-flags-$(call test-ge, $(pahole-ver), 125) += --skip_encoding_btf_inconsis > >> else > >> > >> # Switch to using --btf_features for v1.26 and later. > >> -pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs > >> +pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs,reproducible_build > >> > >> ifneq ($(KBUILD_EXTMOD),) > >> module-pahole-flags-$(call test-ge, $(pahole-ver), 126) += --btf_features=distilled_base > >> > >> Question to the mailing list: do we want this? > > Alan, can you please give us a summary of what are the consequences of > > the reproducible_build pahole option? In terms of performance and > > otherwise. > > > > I've applied patches as is, despite them not solving the issue > > completely, as they are moving us in the right direction anyways. I do > > get slightly different BTF every single time I rebuild my kernel, so > > the change in patch #2 doesn't yet help me. > > > > For libbpf headers, Ihor, can you please follow up with adding > > bpf_helper_defs.h as a dependency? > > > > I have some ideas on how to make BTF regeneration in vmlinux.h itself > > unnecessary, that might help with this issue. Separately (depending on > > what are the negatives of the reproducible_build option) we can look > > into making pahole have more consistent internal BTF type ordering > > without negatively affecting the overall BTF dedup performance in > > pahole. Hopefully I can work with Ihor on this as follow ups. > > > > P.S. I also spent more time than I'm willing to admit trying to > > improve bpftool's BTF sorting to minimize the chance of vmlinux.h > > contents being different, and I think I removed a bunch of cases where > > we had unnecessary differences, but still, it's fundamentally > > non-deterministic to do everything based on type and field names, > > unfortunately. > > > > Anyways, Mykyta (cc'ed), what do you think about the changes below? > > Note that I'm also fixing the incorrect handling of enum64 (would be > > nice to prepare a proper patch and send it upstream, if you get a > > chance). > > [...] > > static struct sort_datum *sort_btf_c(const struct btf *btf) > > @@ -615,8 +680,9 @@ static struct sort_datum *sort_btf_c(const struct btf *btf) > > > > d->index = i; > > d->type_rank = btf_type_rank(btf, i, false); > > - d->sort_name = btf_type_sort_name(btf, i, false); > > + d->sort_name = btf_type_sort_name(btf, i, false, ""); > > d->own_name = btf__name_by_offset(btf, t->name_off); > > + d->disambig_hash = btf_type_disambig_hash(btf, i); > > } > > > > qsort(datums, n, sizeof(struct sort_datum), btf_type_compare); > > > Thanks for pointing to the bug of enum64 handling. I'll create a patch. great, thanks > Reading the rest of the code, hashing struct/union/enum fields is > introduced: > this is only useful for disambiguating ordering of the anonymous > structs/unions/enums. yes, I was trying to have some sort of fixed order. What I was doing was basically: touch kernel/bpf/verifier.c && make -j$(nproc) and after that generated vmlinux.h and compared with the previous version. And every single time it differs a bit due to a) there being types that are named the same, but really are different types and b) as a consequence, different order of types will cause different ___2, ___3 suffixes, which causes tiny changes to vmlinux.h) Reproducible build would help with the ordering, but I was hoping to achieve similar effect with bpftool (and I did remove some of the unnecessary changes, but not all). > > I suspect the biggest source of the issues are structs and unions, though. > Are definitions like this create problems? > typedef struct {...} foo_t; > ? > I'll check what other differences this change makes. just try the kernel rebuild and vmlinux.h generation steps above, you'll see :) > >> [...] > >> >
