Messages in this thread | | | Subject | Re: [PATCH v6 03/20] perf record: Introduce thread local variable | From | Riccardo Mancini <> | Date | Fri, 04 Jun 2021 00:56:48 +0200 |
| |
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);
Thanks, Riccardo
> + if (!quiet) > + fprintf(stderr, "[ perf record: Woken up %ld times to write data > ]\n", waking); > + > if (rec->session->bytes_transferred && rec->session->bytes_compressed) { > ratio = (float)rec->session->bytes_transferred/(float)rec- > >session->bytes_compressed; > session->header.env.comp_ratio = ratio + 0.5; SNIP
| |