Re: [PATCH dwarves v4 3/4] pahole: Use per-thread btf instances to avoid mutex locking.

[Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>]

Em Wed, Jan 26, 2022 at 11:58:27AM -0800, Andrii Nakryiko escreveu:
> On Wed, Jan 26, 2022 at 11:21 AM Kui-Feng Lee <kuifeng@xxxxxx> wrote:
> >
> > Create an instance of btf for each worker thread, and add type info to
> > the local btf instance in the steal-function of pahole without mutex
> > acquiring.  Once finished with all worker threads, merge all
> > per-thread btf instances to the primary btf instance.
> >
> > Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxx>
> > ---
> 
> There are still unnecessary casts and missing {} in the else branch,
> but I'll let Arnaldo decide or fix it up.
> 
> Once this lands, can you please send kernel patch to use -j if pahole
> support it during the kernel build? See scripts/pahole-version.sh and
> scripts/link-vmlinux.sh for how pahole is set up and used in the
> kernel. Thanks!

I also tweaked this:

diff --git a/pahole.c b/pahole.c
index 8dcd6bf951fe1f93..1b2b19b2be45d30c 100644
--- a/pahole.c
+++ b/pahole.c
@@ -2887,13 +2887,13 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
                static pthread_mutex_t btf_lock = PTHREAD_MUTEX_INITIALIZER;
                struct btf_encoder *encoder;

+               pthread_mutex_lock(&btf_lock);
                /*
                 * FIXME:
                 *
                 * This should be really done at main(), but since in the current codebase only at this
                 * point we'll have cu->elf setup...
                 */
-               pthread_mutex_lock(&btf_lock);
                if (!btf_encoder) {
                        /*
                         * btf_encoder is the primary encoder.
⬢[acme@toolbox pahole]$

As moving it to after that comment will only make the patch a bit
larger, changing nothing.

+++ b/pahole.c
@@ -2900,11 +2900,9 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
                         * And, it is used by the thread
                         * create it.
                         */
-                       btf_encoder = btf_encoder__new(cu, detached_btf_filename,
-                                                      conf_load->base_btf,
-                                                      skip_encoding_btf_vars,
-                                                      btf_encode_force,
-                                                      btf_gen_floats, global_verbose);
+                       btf_encoder = btf_encoder__new(cu, detached_btf_filename, conf_load->base_btf, skip_encoding_btf_vars,
+                                                      btf_encode_force, btf_gen_floats, global_verbose);
+
                        if (btf_encoder && thr_data) {
                                struct thread_data *thread = thr_data;

⬢[acme@toolbox pahole]$

i.e. cosmetic stuff to make the patch smaller by keeping preexisting
lines as-is.

And the missing {} Andrii noticed:

diff --git a/pahole.c b/pahole.c
index 7e2e37582f21c566..8c0a982f05c9ae3d 100644
--- a/pahole.c
+++ b/pahole.c
@@ -2937,8 +2937,9 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
                                thread->btf = btf_encoder__btf(thread->encoder);
                        }
                        encoder = thread->encoder;
-               } else
+               } else {
                        encoder = btf_encoder;
+               }

                if (btf_encoder__encode_cu(encoder, cu)) {
                        fprintf(stderr, "Encountered error while encoding BTF.\n");
⬢[acme@toolbox pahole]$

I'll look at the needless casts and push it to the 'next' and tmp.master
branches so that libbpf's CI can have a chance to test it.

I also added 'perf stat' results for 1.21 (the one in this fedora 34
workstation), 1.23 (parallel DWARF loading) and with your patches + the
libbpf update as a committer testing section, please add performance
numbers in in future work.

Thanks, applied!

- Arnaldo