From: Riccardo Mancini <rickyman7@gmail.com>
To: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Andi Kleen <ak@linux.intel.com>,
Adrian Hunter <adrian.hunter@intel.com>,
Alexander Antonov <alexander.antonov@linux.intel.com>,
Alexei Budankov <abudankov@huawei.com>,
linux-perf-users@vger.kernel.org, Ian Rogers <irogers@google.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>
Subject: Re: [PATCH v6 03/20] perf record: Introduce thread local variable
Date: Fri, 04 Jun 2021 00:56:48 +0200 [thread overview]
Message-ID: <3b297a17f935d2a00bfa74afbbf064b01fe83607.camel@gmail.com> (raw)
In-Reply-To: <c5a046f8bed989e4ede98f1fcdaa9d0b6bf78cac.1622025774.git.alexey.v.bayduraev@linux.intel.com>
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
next prev parent reply other threads:[~2021-06-03 22:57 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-26 10:52 [PATCH v6 00/20] Introduce threaded trace streaming for basic perf record operation Alexey Bayduraev
2021-05-26 10:52 ` [PATCH v6 01/20] perf record: Introduce thread affinity and mmap masks Alexey Bayduraev
2021-05-26 10:52 ` [PATCH v6 02/20] perf record: Introduce thread specific data array Alexey Bayduraev
2021-05-26 10:52 ` [PATCH v6 03/20] perf record: Introduce thread local variable Alexey Bayduraev
2021-06-03 22:56 ` Riccardo Mancini [this message]
2021-06-09 22:54 ` Namhyung Kim
2021-05-26 10:52 ` [PATCH v6 04/20] perf record: Stop threads in the end of trace streaming Alexey Bayduraev
2021-05-26 10:52 ` [PATCH v6 05/20] perf record: Start threads in the beginning " Alexey Bayduraev
2021-06-03 23:01 ` Riccardo Mancini
2021-05-26 10:52 ` [PATCH v6 06/20] perf record: Introduce data file at mmap buffer object Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 07/20] perf record: Introduce data transferred and compressed stats Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 08/20] perf record: Init data file at mmap buffer object Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 09/20] tools lib: Introduce bitmap_intersects() operation Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 10/20] perf record: Introduce --threads=<spec> command line option Alexey Bayduraev
2021-06-03 23:14 ` Riccardo Mancini
2021-05-26 10:53 ` [PATCH v6 11/20] perf record: Document parallel data streaming mode Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 12/20] perf report: Output data file name in raw trace dump Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 13/20] perf session: Move reader structure to the top Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 14/20] perf session: Introduce reader_state in reader object Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 15/20] perf session: Introduce reader objects in session object Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 16/20] perf session: Introduce decompressor into trace reader object Alexey Bayduraev
2021-06-03 23:22 ` Riccardo Mancini
2021-05-26 10:53 ` [PATCH v6 17/20] perf session: Move init into reader__init function Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 18/20] perf session: Move map/unmap into reader__mmap function Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 19/20] perf session: Load single file for analysis Alexey Bayduraev
2021-05-26 10:53 ` [PATCH v6 20/20] perf session: Load data directory files " Alexey Bayduraev
2021-06-03 23:28 ` Riccardo Mancini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b297a17f935d2a00bfa74afbbf064b01fe83607.camel@gmail.com \
--to=rickyman7@gmail.com \
--cc=abudankov@huawei.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.antonov@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexey.v.bayduraev@linux.intel.com \
--cc=irogers@google.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).