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

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
	Mark Wielaard <mjw@redhat.com>, Kui-Feng Lee <kuifeng@fb.com>,
	dwarves@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH dwarves v4 3/4] pahole: Use per-thread btf instances to avoid mutex locking.
Date: Tue, 1 Feb 2022 21:16:29 -0300	[thread overview]
Message-ID: <YfnNXZPFRra3kmhb@kernel.org> (raw)
In-Reply-To: <CAEf4BzZJzbRZAKgg=Kjg+G2AmD8-H_Pk9j26umicDVAtyWes+g@mail.gmail.com>

Em Mon, Jan 31, 2022 at 10:56:16PM -0800, Andrii Nakryiko escreveu:
> On Fri, Jan 28, 2022 at 11:52 AM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Thu, Jan 27, 2022 at 05:12:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > 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@fb.com> 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.
> >
> > > > There are still unnecessary casts and missing {} in the else branch,
> > > > but I'll let Arnaldo decide or fix it up.
> >
> > So its just one unneeded cast as thr_data here is just a 'void *':
> >
> > diff --git a/pahole.c b/pahole.c
> > index 8c0a982f05c9ae3d..39e18804100dbfda 100644
> > --- a/pahole.c
> > +++ b/pahole.c
> > @@ -2924,7 +2924,7 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
> >                  * avoids copying the data collected by the first thread.
> >                  */
> >                 if (thr_data) {
> > -                       struct thread_data *thread = (struct thread_data *)thr_data;
> > +                       struct thread_data *thread = thr_data;
> >
> >                         if (thread->encoder == NULL) {
> >                                 thread->encoder =
> >
> >
> > This other is needed as it is a "void **":
> >
> > @@ -2832,7 +2832,7 @@ static int pahole_thread_exit(struct conf_load *conf, void *thr_data)
> >  static int pahole_threads_collect(struct conf_load *conf, int nr_threads, void **thr_data,
> >                                   int error)
> >  {
> > -       struct thread_data **threads = (struct thread_data **)thr_data;
> > +       struct thread_data **threads = thr_data;
> >         int i;
> >         int err = 0;
> >
> >
> > Removing it:
> >
> > /var/home/acme/git/pahole/pahole.c: In function ‘pahole_threads_collect’:
> > /var/home/acme/git/pahole/pahole.c:2835:40: warning: initialization of ‘struct thread_data **’ from incompatible pointer type ‘void **’ [-Wincompatible-pointer-types]
> >  2835 |         struct thread_data **threads = thr_data;
> >       |                                        ^~~~~~~~
> >
> >
> > And I did some more profiling, now the focus should go to elfutils:
> >
> > ⬢[acme@toolbox pahole]$ perf report --no-children -s dso --call-graph none 2> /dev/null | head -20
> > # To display the perf.data header info, please use --header/--header-only options.
> > #
> > #
> > # Total Lost Samples: 0
> > #
> > # Samples: 27K of event 'cycles:u'
> > # Event count (approx.): 27956766207
> > #
> > # Overhead  Shared Object
> > # ........  ...................
> > #
> >     46.70%  libdwarves.so.1.0.0
> >     39.84%  libdw-0.186.so
> >      9.70%  libc-2.33.so
> >      2.14%  libpthread-2.33.so
> >      1.47%  [unknown]
> >      0.09%  ld-2.33.so
> >      0.06%  libelf-0.186.so
> >      0.00%  libcrypto.so.1.1.1l
> >      0.00%  libk5crypto.so.3.1
> > ⬢[acme@toolbox pahole]$
> >
> > $ perf report -g graph,0.5,2 --stdio --no-children -s dso --dso libdw-0.186.so
> >
> 
> [...]
> 
> >
> > #
> > # (Tip: If you have debuginfo enabled, try: perf report -s sym,srcline)
> > #
> >
> > This find_attr thing needs improvements, its a linear search AFAIK, some
> > hashtable could do wonders, I guess.
> >
> > Mark, was this considered at some point?
> 
> This would be a great improvement, yes!
> 
> But strange that you didn't see any BTF-related functions, are you
> sure you profiled the entire DWARF to BTF conversion process? BTF
> encoding is not dominant, but noticeable anyways (e.g., adding unique
> strings to BTF is relatively expensive still).

It appears under the 

> >     46.70%  libdwarves.so.1.0.0

line, since we statically link libbpf. So yeah, it was unfortunate the
way I presented the profiling output, I was just trying to point to what
I think is the low hanging fruit, i.e. optimizing find_attr routines in
libdw-0.186.so.

> >
> > ⬢[acme@toolbox pahole]$ rpm -q elfutils-libs
> > elfutils-libs-0.186-1.fc34.x86_64
> >
> > Andrii https://github.com/libbpf/libbpf/actions/workflows/pahole.yml is
> > in failure mode for 3 days, and only yesterday I pushed these changes,
> > seems unrelated to pahole:
> >
> > Tests exit status: 1
> > Test Results:
> >              bpftool: PASS
> >           test_progs: FAIL (returned 1)
> >  test_progs-no_alu32: FAIL (returned 1)
> >        test_verifier: PASS
> >             shutdown: CLEAN
> > Error: Process completed with exit code 1.
> >
> > Can you please check?
> 
> Yes, it's not related to pahole. This is the BPF selftests issue which
> I already fixed last week, but didn't get a chance to sync everything
> to Github repo before leaving for a short vacation. I'll do another
> sync tonight and it should be all green again.

I'll check tomorrow.

Going green I'll do some more tests and work towards releasing 1.24.

- Arnaldo