Re: [PATCH bpf-next 2/2] selftests/bpf: do not update vmlinux.h unnecessarily

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Fri, Aug 30, 2024 at 10:03:40PM +0100, Alan Maguire wrote:
> On 30/08/2024 21:34, Andrii Nakryiko wrote:
> > On Wed, Aug 28, 2024 at 3:02 PM Eduard Zingerman <eddyz87@gmail.com> 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@mail.gmail.com
> >>>
> >>> Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
> >>> ---
> >>
> >> 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.
> >
> 
> Sure. The original context was that the folks trying to do reproducible
> builds were being impacted by the fact that BTF generation was
> non-deterministic when done in parallel; i.e. same kernel would give
> different BTF ids when rebuilding vmlinux BTF; the reason was largely as
> I understand it that when pahole partitioned CUs between multiple
> threads, that partitioning could vary. If it varied, when BTF was merged
> across threads we could end up with differing id assignments. Since BTF
> then was baked into the vmlinux binary, unstable BTF ids meant
> non-identical vmlinux.
> 
> The first approach to solve this was to remove parallel BTF generation
> to support reproducibility. Arnaldo however added support that retained
> parallelism while supporting determinism through using the DWARF CU
> order. He did some great analysis on the overheads for vmlinux
> generation too [1]; summary is that the overhead in runtime is approx
> 33% versus parallel non-reproducible encoding. Those numbers might not
> 100% translate to the vmlinux build during kernel since it was a
> detached pahole generation and the options might differ slightly, but
> they give a sense of things. I don't _think_ there should be additional
> memory overheads during pahole generation (we really can't afford any
> more memory usage), since it's really more about making order of CU
> processing consistent.
> 
> Would be good to get Arnaldo's perspective too if we're considering
> switching this on by default, as he knows this stuff much better than I do.

You described it nicely! And on top of that there was recent work that
will be available in 1.28 to reduce the memory footprint of pahole,
using it to find things to pack in itself, reducing the number of
allocations, not keeping unreferenced CUs around (you did it), part of
the work to have it working on 32-bit architectures, where we had
reports of it not working.

There is certainly more optimizations to be made to reduce its memory
footprint while allowing it to run in parallel, but at this point it
seems to have addressed the problems that were reported.

More people trying it and measuring the impacts, to confirm the tests
and analysis we did and you alluded too can be only a good thing in
getting us all informed and confortable with using this option by
default.

BTW, we have now a tests/ directory with a regression test for this
feature and another for the --prettify feature in pahole (use DWARF or
BTF to pretty print raw data with several tricks on finding the right
data structure based on enumerations when the conventions used in a
project allow for that, that is the case with tools/perf, also for using
header sizes to traverse variable sized records, etc), please see:

https://git.kernel.org/pub/scm/devel/pahole/pahole.git/tree/tests

And:

https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?id=fd14dc67cb6aaead553074afb4a1ddad10209892
https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?id=be638365781ed0c843249c5bcebe90a01e74b2fe

This should help us in detecting problems earlier, brownie point to
whoever gets this hooked up in CI systems, existing or new 8-)

Thanks,

- Arnaldo
 
> [1]
> https://lore.kernel.org/dwarves/20240412211604.789632-12-acme@kernel.org/