From: Namhyung Kim <namhyung@kernel.org>
To: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Jiri Olsa <jolsa@redhat.com>,
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>,
Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>,
Alexander Antonov <alexander.antonov@linux.intel.com>
Subject: Re: [PATCH v3 02/12] perf record: introduce thread specific data array
Date: Fri, 20 Nov 2020 19:14:14 +0900 [thread overview]
Message-ID: <20201120101414.GC94830@google.com> (raw)
In-Reply-To: <6ad03a01-a3a4-5df4-7cf5-cbc768764e75@linux.intel.com>
On Mon, Nov 16, 2020 at 03:15:42PM +0300, Alexey Budankov wrote:
>
> 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.
>
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
> tools/lib/api/fd/array.c | 17 ++++
> tools/lib/api/fd/array.h | 1 +
> tools/perf/builtin-record.c | 191 +++++++++++++++++++++++++++++++++++-
> 3 files changed, 206 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),
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 82f009703ad7..765a90e38f69 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -56,6 +56,7 @@
> #include <poll.h>
> #include <pthread.h>
> #include <unistd.h>
> +#include <sys/syscall.h>
> #include <sched.h>
> #include <signal.h>
> #ifdef HAVE_EVENTFD_SUPPORT
> @@ -90,6 +91,24 @@ 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];
> + } comm;
I think the name 'comm' is misleading as we have thread's comm
already.
> + struct fdarray pollfd;
> + int ctlfd_pos;
> + struct mmap **maps;
> + struct mmap **overwrite_maps;
> + int nr_mmaps;
> + struct record *rec;
> + unsigned long long samples;
> + unsigned long waking;
> + u64 bytes_written;
> +};
> +
> struct record {
> struct perf_tool tool;
> struct record_opts opts;
> @@ -114,6 +133,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;
> };
>
> @@ -842,9 +862,168 @@ 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_comm(struct thread_data *thread_data)
> +{
> + if (pipe(thread_data->comm.msg) || pipe(thread_data->comm.ack)) {
> + pr_err("Failed to create thread comm pipes, error %m\n");
> + return -ENOMEM;
> + }
> +
> + pr_debug("thread_data[%p]: msg=[%d,%d], ack=[%d,%d]\n", thread_data,
> + thread_data->comm.msg[0], thread_data->comm.msg[1],
> + thread_data->comm.ack[0], thread_data->comm.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;
> + }
> + }
> + pr_debug("thread_data[%p]: nr_mmaps=%d, maps=%p, overwrite_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_debug("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_debug("thread_data[%p]: overwrite_maps[%d] -> overwrite_mmap[%d], cpus[%d]\n",
> + thread_data, tm, m, cpus->map[m]);
I'm afraid this will add too much debug message for verbose=1. Maybe
we can demote some to pr_debug2()?
> + }
> + 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_debug("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;
> +
> + thread_data = zalloc(rec->nr_threads * sizeof(*(rec->thread_data)));
> + if (!thread_data) {
> + pr_err("Failed to allocate thread data\n");
> + return -ENOMEM;
> + }
> +
> + 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;
This and other places that return in the middle will leak the
thread_data.
> + ret = record__thread_data_init_pollfd(&thread_data[t], evlist);
> + if (ret)
> + return ret;
> + if (t) {
> + thread_data[t].tid = -1;
> + ret = record__thread_data_init_comm(&thread_data[t]);
> + if (ret)
> + return ret;
> + thread_data[t].ctlfd_pos = fdarray__add(&thread_data[t].pollfd,
> + thread_data[t].comm.msg[0],
> + POLLIN | POLLERR | POLLHUP,
> + fdarray_flag__nonfilterable);
> + if (thread_data[t].ctlfd_pos < 0)
> + return -ENOMEM;
> + pr_debug("thread_data[%p]: pollfd[%d] <- ctl_fd=%d\n",
> + thread_data, thread_data[t].ctlfd_pos,
> + thread_data[t].comm.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 (ret < 0)
> + return ret;
You should check ctlfd_pos instead.
> + pr_debug("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);
> + }
> + }
> +
> + rec->thread_data = thread_data;
> +
> + return 0;
> +}
> +
> +static int record__free_thread_data(struct record *rec)
> +{
> + int t;
> +
> + for (t = 0; t < rec->nr_threads; t++) {
> + close(rec->thread_data[t].comm.msg[0]);
> + close(rec->thread_data[t].comm.msg[1]);
> + close(rec->thread_data[t].comm.ack[0]);
> + close(rec->thread_data[t].comm.ack[1]);
> + zfree(&rec->thread_data[t].maps);
> + zfree(&rec->thread_data[t].overwrite_maps);
> + fdarray__exit(&rec->thread_data[t].pollfd);
The rec->thread_data might not be set.
Thanks,
Namhyung
> + }
> +
> + zfree(&rec->thread_data);
> +
> + return 0;
> +}
> +
> 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;
> @@ -875,6 +1054,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;
> }
>
> @@ -1845,9 +2032,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> perf_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) {
> @@ -1998,6 +2182,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> record__synthesize_workload(rec, true);
>
> out_child:
> + record__free_thread_data(rec);
> evlist__finalize_ctlfd(rec->evlist);
> record__mmap_read_all(rec, true);
> record__aio_mmap_read_sync(rec);
> --
> 2.24.1
>
>
next prev parent reply other threads:[~2020-11-20 10:14 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-16 12:12 [PATCH v3 00/12] Introduce threaded trace streaming for basic perf record operation Alexey Budankov
2020-11-16 12:14 ` [PATCH v3 01/12] perf record: introduce thread affinity and mmap masks Alexey Budankov
2020-11-20 10:01 ` Namhyung Kim
2020-11-16 12:15 ` [PATCH v3 02/12] perf record: introduce thread specific data array Alexey Budankov
2020-11-20 10:14 ` Namhyung Kim [this message]
2020-11-16 12:16 ` [PATCH v3 03/12] perf record: introduce thread local variable Alexey Budankov
2020-11-20 10:20 ` Namhyung Kim
2020-11-16 12:17 ` [PATCH v3 04/12] perf record: stop threads in the end of trace streaming Alexey Budankov
2020-11-16 12:18 ` [PATCH v3 05/12] perf record: start threads in the beginning " Alexey Budankov
2020-11-16 12:18 ` [PATCH v3 06/12] perf record: introduce data file at mmap buffer object Alexey Budankov
2020-11-20 10:28 ` Namhyung Kim
2020-11-16 12:19 ` [PATCH v3 07/12] perf record: init " Alexey Budankov
2020-11-20 10:49 ` Namhyung Kim
2021-03-01 11:16 ` Bayduraev, Alexey V
2021-03-01 11:44 ` Namhyung Kim
2021-03-01 13:33 ` Bayduraev, Alexey V
2021-03-01 14:20 ` Namhyung Kim
2020-11-16 12:20 ` [PATCH v3 08/12] perf record: introduce --threads=<spec> command line option Alexey Budankov
2020-11-20 11:09 ` Namhyung Kim
2020-11-16 12:21 ` [PATCH v3 09/12] perf record: document parallel data streaming mode Alexey Budankov
2020-11-16 12:22 ` [PATCH v3 10/12] perf report: output data file name in raw trace dump Alexey Budankov
2020-11-16 12:22 ` [PATCH v3 11/12] perf session: load data directory files for analysis Alexey Budankov
2020-11-16 12:25 ` [PATCH v3 12/12] perf session: use reader functions to load perf data file Alexey Budankov
2020-11-20 9:45 ` [PATCH v3 00/12] Introduce threaded trace streaming for basic perf record operation Namhyung Kim
2020-12-15 15:05 ` Alexei Budankov
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=20201120101414.GC94830@google.com \
--to=namhyung@kernel.org \
--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.budankov@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=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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.