Re: [PATCH 02/12] pahole: Disable BTF multithreaded encoded when doing reproducible builds

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Apr 02, 2024 at 04:39:35PM -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Reproducible builds need to produce BTF that have the same ids, which is
> not possible at the moment to do in parallel with libbpf, so serialize
> the encoding.
> 
> The next patches will also make sure that DWARF while being read in
> parallel into internal representation for later BTF encoding has its CU
> (Compile Units) fed to the BTF encoder in the same order as it is in the
> DWARF file, this way we'll produce the same BTF output no matter how
> many threads are used to read BTF.
> 
> Then we'll make sure we have tests in place that compare the output of
> parallel BTF encoding (well, just the DWARF loading part, maybe the BTF
> in the future), i.e. when using 'pahole -j' with the one obtained when
> doing single threaded encoding.
> 
> Testing it on a:
> 
>   # grep -m1 "model name" /proc/cpuinfo
>   model name	: 13th Gen Intel(R) Core(TM) i7-1365U
>   ~#
> 
> I.e. 2 performance cores (4 threads) + 8 efficiency cores.
> 
> From:
> 
>   $ perf stat -r5 pahole -j --btf_encode_detached=vmlinux.btf.parallel vmlinux
> 
>    Performance counter stats for 'pahole -j --btf_encode_detached=vmlinux.btf.parallel vmlinux' (5 runs):
> 
>          17,187.27 msec task-clock:u       #    6.153 CPUs utilized   ( +-  0.34% )
>   <SNIP>
>             2.7931 +- 0.0336 seconds time elapsed  ( +-  1.20% )
> 
>   $
> 
> To:
> 
>   $ perf stat -r5 pahole -j --reproducible_build --btf_encode_detached=vmlinux.btf.parallel.reproducible_build vmlinux
> 
>    Performance counter stats for 'pahole -j --reproducible_build --btf_encode_detached=vmlinux.btf.parallel.reproducible_build vmlinux' (5 runs):
> 
>          14,654.06 msec task-clock:u       #    3.507 CPUs utilized   ( +-  0.45% )
>   <SNIP>
>             4.1787 +- 0.0344 seconds time elapsed  ( +-  0.82% )
> 
>   $
> 
> Which is still a nice improvement over doing it completely serially:
> 
>   $ perf stat -r5 pahole --btf_encode_detached=vmlinux.btf.serial vmlinux
> 
>    Performance counter stats for 'pahole --btf_encode_detached=vmlinux.btf.serial vmlinux' (5 runs):
> 
>           7,506.93 msec task-clock:u       #    1.000 CPUs utilized   ( +-  0.13% )
>   <SNIP>
>             7.5106 +- 0.0115 seconds time elapsed  ( +-  0.15% )
> 
>   $
> 
>   $ pahole vmlinux.btf.parallel > /tmp/parallel
>   $ pahole vmlinux.btf.parallel.reproducible_build > /tmp/parallel.reproducible_build
>   $ diff -u /tmp/parallel /tmp/parallel.reproducible_build | wc -l
>   269920
>   $ pahole --sort vmlinux.btf.parallel > /tmp/parallel.sorted
>   $ pahole --sort vmlinux.btf.parallel.reproducible_build > /tmp/parallel.reproducible_build.sorted
>   $ diff -u /tmp/parallel.sorted /tmp/parallel.reproducible_build.sorted | wc -l
>   0
>   $
> 
> The BTF ids continue to be undeterministic, as we need to process the
> CUs (compile unites) in the same order that they are on vmlinux:
> 
>   $ bpftool btf dump file vmlinux.btf.serial > btfdump.serial
>   $ bpftool btf dump file vmlinux.btf.parallel.reproducible_build > btfdump.parallel.reproducible_build
>   $ bpftool btf dump file vmlinux.btf.parallel > btfdump.parallel
>   $ diff -u btfdump.serial btfdump.parallel | wc -l
>   624144
>   $ diff -u btfdump.serial btfdump.parallel.reproducible_build | wc -l
>   594622
>   $ diff -u btfdump.parallel.reproducible_build btfdump.parallel | wc -l
>   623355
>   $
> 
> The BTF ids don't match, we'll get them to match at the end of this
> patch series:
> 
>   $ tail -5 btfdump.serial
>   	type_id=127124 offset=219200 size=40 (VAR 'rt6_uncached_list')
>   	type_id=11760 offset=221184 size=64 (VAR 'vmw_steal_time')
>   	type_id=13533 offset=221248 size=8 (VAR 'kvm_apic_eoi')
>   	type_id=13532 offset=221312 size=64 (VAR 'steal_time')
>   	type_id=13531 offset=221376 size=68 (VAR 'apf_reason')
>   $ tail -5 btfdump.parallel.reproducible_build
>   	type_id=113812 offset=219200 size=40 (VAR 'rt6_uncached_list')
>   	type_id=87979 offset=221184 size=64 (VAR 'vmw_steal_time')
>   	type_id=127391 offset=221248 size=8 (VAR 'kvm_apic_eoi')
>   	type_id=127390 offset=221312 size=64 (VAR 'steal_time')
>   	type_id=127389 offset=221376 size=68 (VAR 'apf_reason')
>   $
> 
> Now to make it process the CUs in order, that should get everything
> straight without hopefully not degrading it further too much.
> 
> Cc: Alan Maguire <alan.maguire@oracle.com>
> Cc: Kui-Feng Lee <kuifeng@fb.com>
> Cc: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  pahole.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/pahole.c b/pahole.c
> index 96e153432fa212a5..fcb4360f11debeb9 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -3173,6 +3173,14 @@ struct thread_data {
>  	struct btf_encoder *encoder;
>  };
>  
> +static int pahole_threads_prepare_reproducible_build(struct conf_load *conf, int nr_threads, void **thr_data)
> +{
> +	for (int i = 0; i < nr_threads; i++)
> +		thr_data[i] = NULL;
> +
> +	return 0;
> +}
> +
>  static int pahole_threads_prepare(struct conf_load *conf, int nr_threads, void **thr_data)
>  {
>  	int i;
> @@ -3283,7 +3291,10 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
>  				thread->btf = btf_encoder__btf(btf_encoder);
>  			}
>  		}
> -		pthread_mutex_unlock(&btf_lock);
> +
> +		// Reproducible builds don't have multiple btf_encoders, so we need to keep the lock until we encode BTF for this CU.
> +		if (thr_data)
> +			pthread_mutex_unlock(&btf_lock);

so the idea is that this code is executed in threads but with
NULL in thr_data , right?

>  
>  		if (!btf_encoder) {
>  			ret = LSK__STOP_LOADING;
> @@ -3319,6 +3330,8 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
>  			exit(1);
>  		}
>  out_btf:
> +		if (!thr_data) // See comment about reproducibe_build above
> +			pthread_mutex_unlock(&btf_lock);
>  		return ret;
>  	}
>  #if 0
> @@ -3689,8 +3702,14 @@ int main(int argc, char *argv[])
>  
>  	conf_load.steal = pahole_stealer;
>  	conf_load.thread_exit = pahole_thread_exit;
> -	conf_load.threads_prepare = pahole_threads_prepare;
> -	conf_load.threads_collect = pahole_threads_collect;
> +
> +	if (conf_load.reproducible_build) {
> +		conf_load.threads_prepare = pahole_threads_prepare_reproducible_build;

would it be enough just to set conf_load.threads_prepare to NULL? 

there's memset in dwarf_cus__threaded_process_cus doing the same
thing as pahole_threads_prepare_reproducible_build

jirka

> +		conf_load.threads_collect = NULL;
> +	} else {
> +		conf_load.threads_prepare = pahole_threads_prepare;
> +		conf_load.threads_collect = pahole_threads_collect;
> +	}
>  
>  	// Make 'pahole --header type < file' a shorter form of 'pahole -C type --count 1 < file'
>  	if (conf.header_type && !class_name && prettify_input) {
> -- 
> 2.44.0
>