[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAM9d7cjJKDoMrKTzuCK=zDZivTdT-o30JKPt=97D2JEnXaHr0w@mail.gmail.com>
Date: Wed, 9 Jun 2021 15:54:23 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Riccardo Mancini <rickyman7@...il.com>
Cc: Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>,
Jiri Olsa <jolsa@...hat.com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Andi Kleen <ak@...ux.intel.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Alexander Antonov <alexander.antonov@...ux.intel.com>,
Alexei Budankov <abudankov@...wei.com>,
linux-perf-users <linux-perf-users@...r.kernel.org>,
Ian Rogers <irogers@...gle.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>
Subject: Re: [PATCH v6 03/20] perf record: Introduce thread local variable
Hi Riccardo,
On Thu, Jun 3, 2021 at 3:56 PM Riccardo Mancini <rickyman7@...il.com> wrote:
>
> Hi,
>
> thank you very much for your work for adding threading capabilites to perf
> record.
> I did some testing on your entire patchset, especially checking for memory
> issues using ASan. This is just the first of a couple of emails to point out
> some issues I found.
> I will also do additional tests in the future.
>
> On Wed, 2021-05-26 at 13:52 +0300, Alexey Bayduraev wrote:
> SNIP
> > @@ -2220,18 +2275,20 @@ static int __cmd_record(struct record *rec, int argc,
> > const char **argv)
> > goto out_child;
> > }
> >
> > - if (!quiet)
> > - fprintf(stderr, "[ perf record: Woken up %ld times to write data
> > ]\n", waking);
> > -
> > if (target__none(&rec->opts.target))
> > record__synthesize_workload(rec, true);
> >
> > out_child:
> > + record__stop_threads(rec, &waking);
> > +out_free_threads:
> > record__free_thread_data(rec);
> > evlist__finalize_ctlfd(rec->evlist);
> > record__mmap_read_all(rec, true);
> > record__aio_mmap_read_sync(rec);
>
> record__mmap_read_all should be moved before record__free_thread_data since it
> uses the thread_data that's just been freed.
> Furthermore, record__mmap_read_all should also be moved before the
> out_free_threads label, since it cannot be called unless record__start_threads
> succeeded, otherwise thread would be NULL and will cause a segfault (it happens
> if there is an error somewhere else in perf, for example).
>
> In my tests the following order works, but it should be double checked for
> possible side-effects of this order change.
>
> out_child:
> record__stop_threads(rec, &waking);
> record__mmap_read_all(rec, true);
> out_free_threads:
> record__free_thread_data(rec);
> evlist__finalize_ctlfd(rec->evlist);
> record__aio_mmap_read_sync(rec);
I wonder how it worked before.. maybe we should place
record__free_thread_data() far below.
Thanks,
Namhyung
Powered by blists - more mailing lists