From: David Ahern <dsahern@gmail.com>
To: chenggang <chenggang.qin@gmail.com>
Cc: linux-kernel@vger.kernel.org,
chenggang <chenggang.qcg@taobao.com>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@ghostprotocols.net>,
Arjan van de Ven <arjan@linux.intel.com>,
Namhyung Kim <namhyung@gmail.com>,
Yanmin Zhang <yanmin.zhang@intel.com>,
Wu Fengguang <fengguang.wu@intel.com>,
Mike Galbraith <efault@gmx.de>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v3 1/8]Perf: Transform thread_map to linked list
Date: Sun, 17 Mar 2013 17:37:43 -0600 [thread overview]
Message-ID: <514653C7.2020203@gmail.com> (raw)
In-Reply-To: <1363167740-27735-8-git-send-email-chenggang.qin@gmail.com>
Hi:
On 3/13/13 3:42 AM, chenggang wrote:
> ---
> tools/perf/builtin-stat.c | 2 +-
> tools/perf/tests/open-syscall-tp-fields.c | 2 +-
> tools/perf/util/event.c | 12 +-
> tools/perf/util/evlist.c | 2 +-
> tools/perf/util/evsel.c | 16 +-
> tools/perf/util/python.c | 2 +-
> tools/perf/util/thread_map.c | 281 ++++++++++++++++++++++-------
> tools/perf/util/thread_map.h | 17 +-
> 8 files changed, 244 insertions(+), 90 deletions(-)
You need to target smaller changes per patch. Think small, bisectable
changes that evolve the code to where you want it to be.
For example, start with a patch that introduces the API
thread_map__set_pid_by_idx:
int thread_map__set_pid_by_idx(struct thread_map *map, int idx, pid_t pid)
{
if (idx >= map->nr)
return -EINVAL;
map[idx] = pid;
return 0;
}
and use that method for the perf-stat change,
tests/open-syscall-tp-fields.c and util/evlist.c. That's patch 1 --
small and focused.
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 9984876..293b09c 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -401,7 +401,7 @@ static int __run_perf_stat(int argc __maybe_unused, const char **argv)
> }
>
> if (perf_target__none(&target))
> - evsel_list->threads->map[0] = child_pid;
> + thread_map__set_pid(evsel_list->threads, 0, child_pid);
>
> /*
> * Wait for the child to be ready to exec.
> diff --git a/tools/perf/tests/open-syscall-tp-fields.c b/tools/perf/tests/open-syscall-tp-fields.c
> index 1c52fdc..39eb770 100644
> --- a/tools/perf/tests/open-syscall-tp-fields.c
> +++ b/tools/perf/tests/open-syscall-tp-fields.c
> @@ -43,7 +43,7 @@ int test__syscall_open_tp_fields(void)
>
> perf_evsel__config(evsel, &opts);
>
> - evlist->threads->map[0] = getpid();
> + thread_map__append(evlist->threads, getpid());
>
> err = perf_evlist__open(evlist);
> if (err < 0) {
The second small patch introduces the method thread_map__get_pid_by_idx
which is the counterpart to thread_map__set_pid_by_idx -- this time
returning the pid for a given index. This new method fixes this use in
util/event.c and the one in python.c below.
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 5cd13d7..d093460 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -326,9 +326,11 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
>
> err = 0;
> for (thread = 0; thread < threads->nr; ++thread) {
> + pid_t pid = thread_map__get_pid(threads, thread);
> +
> if (__event__synthesize_thread(comm_event, mmap_event,
> - threads->map[thread], 0,
> - process, tool, machine)) {
> + pid, 0, process, tool,
> + machine)) {
> err = -1;
> break;
> }
> @@ -337,12 +339,14 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
> * comm.pid is set to thread group id by
> * perf_event__synthesize_comm
> */
> - if ((int) comm_event->comm.pid != threads->map[thread]) {
> + if ((int) comm_event->comm.pid != pid) {
> bool need_leader = true;
>
> /* is thread group leader in thread_map? */
> for (j = 0; j < threads->nr; ++j) {
> - if ((int) comm_event->comm.pid == threads->map[j]) {
> + pid_t pidj = thread_map__get_pid(threads, j);
> +
> + if ((int) comm_event->comm.pid == pidj) {
> need_leader = false;
> break;
> }
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index bc4ad79..d5063d6 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -793,7 +793,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
> }
>
> if (perf_target__none(&opts->target))
> - evlist->threads->map[0] = evlist->workload.pid;
> + thread_map__append(evlist->threads, evlist->workload.pid);
Here you can use thread_map__set_pid_by_idx. When you convert the
xyarray implementation you can come back to this call and change it to
thread_map__append or have set_pid_by_idx do the append internally if
the idx == threads->nr
>
> close(child_ready_pipe[1]);
> close(go_pipe[0]);
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 9c82f98f..57c569d 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -835,7 +835,7 @@ retry_sample_id:
> int group_fd;
>
> if (!evsel->cgrp)
> - pid = threads->map[thread];
> + pid = thread_map__get_pid(threads, thread);
>
> group_fd = get_group_fd(evsel, cpu, thread);
>
This part here can be a separate stand-alone patch. In thread_map.c
introduce the method:
struct thread_map *thread_map__empty_thread_map(void)
{
static struct {
struct thread_map map;
int threads[1];
} empty_thread_map = {
.map.nr = 1,
.threads = { -1, },
};
return &empty_thread_map.map;
}
In a follow up patch you can change the implementation of this method
but for now you want a small bisectable change.
> @@ -894,14 +894,6 @@ static struct {
> .cpus = { -1, },
> };
>
> -static struct {
> - struct thread_map map;
> - int threads[1];
> -} empty_thread_map = {
> - .map.nr = 1,
> - .threads = { -1, },
> -};
> -
keep the above code removal and fix the 2 below fixes.
> int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
> struct thread_map *threads)
> {
> @@ -911,7 +903,7 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
> }
>
> if (threads == NULL)
> - threads = &empty_thread_map.map;
> + threads = thread_map__empty_thread_map();
>
> return __perf_evsel__open(evsel, cpus, threads);
> }
> @@ -919,7 +911,9 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
> int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
> struct cpu_map *cpus)
> {
> - return __perf_evsel__open(evsel, cpus, &empty_thread_map.map);
> + struct thread_map *empty_thread_map = thread_map__empty_thread_map();
> +
> + return __perf_evsel__open(evsel, cpus, empty_thread_map);
> }
>
> int perf_evsel__open_per_thread(struct perf_evsel *evsel,
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 925e0c3..e3f3f1b 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -458,7 +458,7 @@ static PyObject *pyrf_thread_map__item(PyObject *obj, Py_ssize_t i)
> if (i >= pthreads->threads->nr)
> return NULL;
>
> - return Py_BuildValue("i", pthreads->threads->map[i]);
> + return Py_BuildValue("i", thread_map__get_pid(pthreads->threads, i));
> }
>
> static PySequenceMethods pyrf_thread_map__sequence_methods = {
Once existing uses of threads->map are consolidated you can create a
patch to convert the thread_map code to xyarray and introduce new
methods needed.
> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
> index 9b5f856..301f4ce 100644
> --- a/tools/perf/util/thread_map.c
> +++ b/tools/perf/util/thread_map.c
> @@ -19,9 +19,116 @@ static int filter(const struct dirent *dir)
> return 1;
> }
>
Does that make sense?
David
next prev parent reply other threads:[~2013-03-17 23:37 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-13 9:42 [PATCH v3 8/8]Perf: Add some callback functions to process fork & exit events chenggang
2013-03-13 9:42 ` [PATCH v3 7/8]Perf: changed the method to traverse mmap list chenggang
2013-03-13 9:42 ` [PATCH v3 6/8]Perf: Add extend mechanism for mmap & pollfd chenggang
2013-03-13 9:42 ` [PATCH v3 5/8]Perf: add extend mechanism for evsel->id & evsel->fd chenggang
2013-03-13 9:42 ` [PATCH v3 4/8]perf: Transform evsel->id to xyarray chenggang
2013-03-17 23:45 ` David Ahern
2013-03-13 9:42 ` [PATCH v3 3/8]Perf: Transform evlist->mmap " chenggang
2013-03-17 23:42 ` David Ahern
2013-03-13 9:42 ` [PATCH v3 2/8]Perf: Transform xyarray to linked list chenggang
2013-03-13 9:42 ` [PATCH v3 1/8]Perf: Transform thread_map " chenggang
2013-03-17 23:37 ` David Ahern [this message]
2013-03-13 9:42 ` [PATCH v3 0/8]Perf: Make the 'perf top -p $pid' can perceive the new forked threads chenggang
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=514653C7.2020203@gmail.com \
--to=dsahern@gmail.com \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@ghostprotocols.net \
--cc=akpm@linux-foundation.org \
--cc=arjan@linux.intel.com \
--cc=chenggang.qcg@taobao.com \
--cc=chenggang.qin@gmail.com \
--cc=efault@gmx.de \
--cc=fengguang.wu@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@gmail.com \
--cc=paulus@samba.org \
--cc=yanmin.zhang@intel.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).