Messages in this thread | | | From | Namhyung Kim <> | Date | Wed, 9 Jun 2021 15:54:23 -0700 | 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@gmail.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
| |