From: Arnaldo Carvalho de Melo <acme@kernel.org>
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>,
Riccardo Mancini <rickyman7@gmail.com>
Subject: Re: [PATCH v8 02/22] perf record: Introduce thread specific data array
Date: Wed, 30 Jun 2021 13:26:09 -0300 [thread overview]
Message-ID: <YNybIfGcP3pj7n8y@kernel.org> (raw)
In-Reply-To: <54085f942fb8deedc617732b4716cb85a5c6ebfb.1625065643.git.alexey.v.bayduraev@linux.intel.com>
Em Wed, Jun 30, 2021 at 06:54:41PM +0300, Alexey Bayduraev escreveu:
> Introduce thread specific data object and array of such objects
> to store and manage thread local data. Implement functions to
> allocate, initialize, finalize and release thread specific data.
>
> Thread local maps and overwrite_maps arrays keep pointers to
> mmap buffer objects to serve according to maps thread mask.
> Thread local pollfd array keeps event fds connected to mmaps
> buffers according to maps thread mask.
>
> Thread control commands are delivered via thread local comm pipes
> and ctlfd_pos fd. External control commands (--control option)
> are delivered via evlist ctlfd_pos fd and handled by the main
> tool thread.
>
> Acked-by: Namhyung Kim <namhyung@gmail.com>
> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
> ---
> tools/lib/api/fd/array.c | 17 ++++
> tools/lib/api/fd/array.h | 1 +
> tools/perf/builtin-record.c | 196 +++++++++++++++++++++++++++++++++++-
> 3 files changed, 211 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 5e6cb9debe37..de8bcbaea3f1 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -88,6 +88,23 @@ int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_flags
> return pos;
> }
>
> +int fdarray__clone(struct fdarray *fda, int pos, struct fdarray *base)
> +{
> + struct pollfd *entry;
> + int npos;
> +
> + if (pos >= base->nr)
> + return -EINVAL;
> +
> + entry = &base->entries[pos];
> +
> + npos = fdarray__add(fda, entry->fd, entry->events, base->priv[pos].flags);
> + if (npos >= 0)
> + fda->priv[npos] = base->priv[pos];
> +
> + return npos;
> +}
> +
> int fdarray__filter(struct fdarray *fda, short revents,
> void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> void *arg)
> diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
> index 7fcf21a33c0c..4a03da7f1fc1 100644
> --- a/tools/lib/api/fd/array.h
> +++ b/tools/lib/api/fd/array.h
> @@ -42,6 +42,7 @@ struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow);
> void fdarray__delete(struct fdarray *fda);
>
> int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_flags flags);
> +int fdarray__clone(struct fdarray *fda, int pos, struct fdarray *base);
> int fdarray__poll(struct fdarray *fda, int timeout);
> int fdarray__filter(struct fdarray *fda, short revents,
> void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
Please split the fdarray.[ch] parts into a separate patch, then the rest
users it in a second patch.
If, theoretically, we needed to revert the builtin-record.c we could do
it with 'git revert' instead of a patch that removes it while leaving
the fdarray__clone part, that at the time of the revert could alredy be
in use in other parts of the code.
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 31b3a515abc1..11ce64b23db4 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -58,6 +58,7 @@
> #include <poll.h>
> #include <pthread.h>
> #include <unistd.h>
> +#include <sys/syscall.h>
> #include <sched.h>
> #include <signal.h>
> #ifdef HAVE_EVENTFD_SUPPORT
> @@ -92,6 +93,23 @@ struct thread_mask {
> struct mmap_cpu_mask affinity;
> };
>
> +struct thread_data {
> + pid_t tid;
> + struct thread_mask *mask;
> + struct {
> + int msg[2];
> + int ack[2];
> + } pipes;
> + struct fdarray pollfd;
> + int ctlfd_pos;
> + struct mmap **maps;
> + struct mmap **overwrite_maps;
> + int nr_mmaps;
Move nr_mmaps to after ctlfd_pos
> + struct record *rec;
> + unsigned long long samples;
> + unsigned long waking;
> +};
> +
> struct record {
> struct perf_tool tool;
> struct record_opts opts;
> @@ -117,6 +135,7 @@ struct record {
> struct mmap_cpu_mask affinity_mask;
> unsigned long output_max_size; /* = 0: unlimited */
> struct thread_mask *thread_masks;
> + struct thread_data *thread_data;
> int nr_threads;
> };
>
> @@ -847,9 +866,174 @@ static int record__kcore_copy(struct machine *machine, struct perf_data *data)
> return kcore_copy(from_dir, kcore_dir);
> }
>
> +static int record__thread_data_init_pipes(struct thread_data *thread_data)
> +{
> + if (pipe(thread_data->pipes.msg) || pipe(thread_data->pipes.ack)) {
> + pr_err("Failed to create thread communication pipes: %s\n", strerror(errno));
> + return -ENOMEM;
> + }
If one fails you should cleanup the other
> +
> + pr_debug2("thread_data[%p]: msg=[%d,%d], ack=[%d,%d]\n", thread_data,
> + thread_data->pipes.msg[0], thread_data->pipes.msg[1],
> + thread_data->pipes.ack[0], thread_data->pipes.ack[1]);
> +
> + return 0;
> +}
> +
> +static int record__thread_data_init_maps(struct thread_data *thread_data, struct evlist *evlist)
> +{
> + int m, tm, nr_mmaps = evlist->core.nr_mmaps;
> + struct mmap *mmap = evlist->mmap;
> + struct mmap *overwrite_mmap = evlist->overwrite_mmap;
> + struct perf_cpu_map *cpus = evlist->core.cpus;
> +
> + thread_data->nr_mmaps = bitmap_weight(thread_data->mask->maps.bits,
> + thread_data->mask->maps.nbits);
> + if (mmap) {
> + thread_data->maps = zalloc(thread_data->nr_mmaps * sizeof(struct mmap *));
> + if (!thread_data->maps) {
> + pr_err("Failed to allocate maps thread data\n");
> + return -ENOMEM;
> + }
> + }
> + if (overwrite_mmap) {
> + thread_data->overwrite_maps = zalloc(thread_data->nr_mmaps * sizeof(struct mmap *));
> + if (!thread_data->overwrite_maps) {
> + pr_err("Failed to allocate overwrite maps thread data\n");
> + return -ENOMEM;
> + }
ditto, release the allocated resources on error exit
> + }
> + pr_debug2("thread_data[%p]: nr_mmaps=%d, maps=%p, ow_maps=%p\n", thread_data,
> + thread_data->nr_mmaps, thread_data->maps, thread_data->overwrite_maps);
> +
> + for (m = 0, tm = 0; m < nr_mmaps && tm < thread_data->nr_mmaps; m++) {
> + if (test_bit(cpus->map[m], thread_data->mask->maps.bits)) {
> + if (thread_data->maps) {
> + thread_data->maps[tm] = &mmap[m];
> + pr_debug2("thread_data[%p]: maps[%d] -> mmap[%d], cpus[%d]\n",
> + thread_data, tm, m, cpus->map[m]);
> + }
> + if (thread_data->overwrite_maps) {
> + thread_data->overwrite_maps[tm] = &overwrite_mmap[m];
> + pr_debug2("thread_data[%p]: ow_maps[%d] -> ow_mmap[%d], cpus[%d]\n",
> + thread_data, tm, m, cpus->map[m]);
> + }
> + tm++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int record__thread_data_init_pollfd(struct thread_data *thread_data, struct evlist *evlist)
> +{
> + int f, tm, pos;
> + struct mmap *map, *overwrite_map;
> +
> + fdarray__init(&thread_data->pollfd, 64);
> +
> + for (tm = 0; tm < thread_data->nr_mmaps; tm++) {
> + map = thread_data->maps ? thread_data->maps[tm] : NULL;
> + overwrite_map = thread_data->overwrite_maps ?
> + thread_data->overwrite_maps[tm] : NULL;
> +
> + for (f = 0; f < evlist->core.pollfd.nr; f++) {
> + void *ptr = evlist->core.pollfd.priv[f].ptr;
> +
> + if ((map && ptr == map) || (overwrite_map && ptr == overwrite_map)) {
> + pos = fdarray__clone(&thread_data->pollfd, f, &evlist->core.pollfd);
> + if (pos < 0)
> + return pos;
> + pr_debug2("thread_data[%p]: pollfd[%d] <- event_fd=%d\n",
> + thread_data, pos, evlist->core.pollfd.entries[f].fd);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int record__alloc_thread_data(struct record *rec, struct evlist *evlist)
> +{
> + int t, ret;
> + struct thread_data *thread_data;
> +
> + rec->thread_data = zalloc(rec->nr_threads * sizeof(*(rec->thread_data)));
> + if (!rec->thread_data) {
> + pr_err("Failed to allocate thread data\n");
> + return -ENOMEM;
> + }
> + thread_data = rec->thread_data;
> +
> + for (t = 0; t < rec->nr_threads; t++) {
> + thread_data[t].rec = rec;
> + thread_data[t].mask = &rec->thread_masks[t];
> + ret = record__thread_data_init_maps(&thread_data[t], evlist);
> + if (ret)
> + return ret;
Also release allocated resources on exit
> + ret = record__thread_data_init_pollfd(&thread_data[t], evlist);
So record__thread_data_init_pollfd() can fail, you emitted a warning
here for zalloc() failure, and in the record__thread_data_init_maps()
case that function emits error messages, for consistency please emit one
here or in record__thread_data_init_pollfd() when fdarray__clone() fails
> + if (ret)
> + return ret;
release resources on exit
> + if (t) {
> + thread_data[t].tid = -1;
> + ret = record__thread_data_init_pipes(&thread_data[t]);
> + if (ret)
> + return ret;
release resources on exit
> + thread_data[t].ctlfd_pos = fdarray__add(&thread_data[t].pollfd,
> + thread_data[t].pipes.msg[0],
> + POLLIN | POLLERR | POLLHUP,
> + fdarray_flag__nonfilterable);
> + if (thread_data[t].ctlfd_pos < 0)
> + return -ENOMEM;
pr_err() and release resources
> + pr_debug2("thread_data[%p]: pollfd[%d] <- ctl_fd=%d\n",
> + thread_data, thread_data[t].ctlfd_pos,
> + thread_data[t].pipes.msg[0]);
> + } else {
> + thread_data[t].tid = syscall(SYS_gettid);
> + if (evlist->ctl_fd.pos == -1)
> + continue;
> + thread_data[t].ctlfd_pos = fdarray__clone(&thread_data[t].pollfd,
> + evlist->ctl_fd.pos,
> + &evlist->core.pollfd);
> + if (thread_data[t].ctlfd_pos < 0)
> + return -ENOMEM;
Ditto
> + pr_debug2("thread_data[%p]: pollfd[%d] <- ctl_fd=%d\n",
> + thread_data, thread_data[t].ctlfd_pos,
> + evlist->core.pollfd.entries[evlist->ctl_fd.pos].fd);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void record__free_thread_data(struct record *rec)
> +{
> + int t;
> +
> + if (rec->thread_data == NULL)
> + return;
> +
> + for (t = 0; t < rec->nr_threads; t++) {
> + if (rec->thread_data[t].pipes.msg[0])
> + close(rec->thread_data[t].pipes.msg[0]);
Just to make this consistent with the zfree() use below, please init
rec->thread_data[t].pipes.msg[0] to zero (probably best would be to -1)
> + if (rec->thread_data[t].pipes.msg[1])
> + close(rec->thread_data[t].pipes.msg[1]);
> + if (rec->thread_data[t].pipes.ack[0])
> + close(rec->thread_data[t].pipes.ack[0]);
> + if (rec->thread_data[t].pipes.ack[1])
> + close(rec->thread_data[t].pipes.ack[1]);
> + zfree(&rec->thread_data[t].maps);
> + zfree(&rec->thread_data[t].overwrite_maps);
> + fdarray__exit(&rec->thread_data[t].pollfd);
> + }
> +
> + zfree(&rec->thread_data);
> +}
> +
> static int record__mmap_evlist(struct record *rec,
> struct evlist *evlist)
> {
> + int ret;
> struct record_opts *opts = &rec->opts;
> bool auxtrace_overwrite = opts->auxtrace_snapshot_mode ||
> opts->auxtrace_sample_mode;
> @@ -880,6 +1064,14 @@ static int record__mmap_evlist(struct record *rec,
> return -EINVAL;
> }
> }
> +
> + if (evlist__initialize_ctlfd(evlist, opts->ctl_fd, opts->ctl_fd_ack))
> + return -1;
> +
> + ret = record__alloc_thread_data(rec, evlist);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> @@ -1880,9 +2072,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> evlist__start_workload(rec->evlist);
> }
>
> - if (evlist__initialize_ctlfd(rec->evlist, opts->ctl_fd, opts->ctl_fd_ack))
> - goto out_child;
> -
> if (opts->initial_delay) {
> pr_info(EVLIST_DISABLED_MSG);
> if (opts->initial_delay > 0) {
> @@ -2040,6 +2229,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> out_child:
> evlist__finalize_ctlfd(rec->evlist);
> record__mmap_read_all(rec, true);
> + record__free_thread_data(rec);
> record__aio_mmap_read_sync(rec);
>
> if (rec->session->bytes_transferred && rec->session->bytes_compressed) {
> --
> 2.19.0
>
--
- Arnaldo
next prev parent reply other threads:[~2021-06-30 16:26 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-30 15:54 [PATCH v8 00/22] Introduce threaded trace streaming for basic perf record operation Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 01/22] perf record: Introduce thread affinity and mmap masks Alexey Bayduraev
2021-06-30 16:17 ` Arnaldo Carvalho de Melo
2021-06-30 16:28 ` Arnaldo Carvalho de Melo
2021-07-01 13:05 ` Bayduraev, Alexey V
2021-07-01 14:23 ` Arnaldo Carvalho de Melo
2021-06-30 15:54 ` [PATCH v8 02/22] perf record: Introduce thread specific data array Alexey Bayduraev
2021-06-30 16:26 ` Arnaldo Carvalho de Melo [this message]
2021-06-30 17:18 ` Arnaldo Carvalho de Melo
2021-06-30 15:54 ` [PATCH v8 03/22] perf record: Introduce thread local variable Alexey Bayduraev
2021-06-30 17:16 ` Arnaldo Carvalho de Melo
2021-07-01 17:22 ` Bayduraev, Alexey V
2021-06-30 15:54 ` [PATCH v8 04/22] perf record: Stop threads in the end of trace streaming Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 05/22] perf record: Start threads in the beginning " Alexey Bayduraev
2021-06-30 17:21 ` Arnaldo Carvalho de Melo
2021-06-30 15:54 ` [PATCH v8 06/22] perf record: Introduce data file at mmap buffer object Alexey Bayduraev
2021-06-30 17:23 ` Arnaldo Carvalho de Melo
2021-07-01 16:41 ` Bayduraev, Alexey V
2021-07-01 17:28 ` Arnaldo Carvalho de Melo
2021-06-30 15:54 ` [PATCH v8 07/22] perf record: Introduce data transferred and compressed stats Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 08/22] perf record: Init data file at mmap buffer object Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 09/22] tools lib: Introduce bitmap_intersects() operation Alexey Bayduraev
2021-06-30 17:24 ` Arnaldo Carvalho de Melo
2021-06-30 17:33 ` Bayduraev, Alexey V
2021-06-30 17:42 ` Arnaldo Carvalho de Melo
2021-06-30 15:54 ` [PATCH v8 10/22] perf record: Introduce --threads=<spec> command line option Alexey Bayduraev
2021-06-30 17:28 ` Arnaldo Carvalho de Melo
2021-06-30 18:54 ` Bayduraev, Alexey V
2021-07-01 11:50 ` Bayduraev, Alexey V
2021-07-01 14:26 ` Arnaldo Carvalho de Melo
2021-07-01 18:14 ` Bayduraev, Alexey V
2021-06-30 15:54 ` [PATCH v8 11/22] perf record: Document parallel data streaming mode Alexey Bayduraev
2021-06-30 17:28 ` Arnaldo Carvalho de Melo
2021-06-30 15:54 ` [PATCH v8 12/22] perf report: Output data file name in raw trace dump Alexey Bayduraev
2021-06-30 18:36 ` Arnaldo Carvalho de Melo
2021-07-01 22:46 ` Bayduraev, Alexey V
2021-06-30 15:54 ` [PATCH v8 13/22] perf session: Move reader structure to the top Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 14/22] perf session: Introduce reader_state in reader object Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 15/22] perf session: Introduce reader objects in session object Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 16/22] perf session: Introduce decompressor into trace reader object Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 17/22] perf session: Move init into reader__init function Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 18/22] perf session: Move map/unmap into reader__mmap function Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 19/22] perf session: Load single file for analysis Alexey Bayduraev
2021-06-30 15:54 ` [PATCH v8 20/22] perf session: Load data directory files " Alexey Bayduraev
2021-07-02 10:30 ` Jiri Olsa
2021-07-02 12:04 ` Bayduraev, Alexey V
2021-06-30 15:55 ` [PATCH v8 21/22] perf session: Introduce READER_NODATA state Alexey Bayduraev
2021-07-01 10:08 ` Bayduraev, Alexey V
2021-06-30 15:55 ` [PATCH v8 22/22] perf record: Introduce record__bytes_written and fix --max-size option Alexey Bayduraev
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=YNybIfGcP3pj7n8y@kernel.org \
--to=acme@kernel.org \
--cc=abudankov@huawei.com \
--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=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=rickyman7@gmail.com \
/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).