| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <514653C7.2020203@gmail.com>
Date: Sun, 17 Mar 2013 17:37:43 -0600
From: David Ahern <dsahern@...il.com>
To: chenggang <chenggang.qin@...il.com>
CC: linux-kernel@...r.kernel.org, chenggang <chenggang.qcg@...bao.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Paul Mackerras <paulus@...ba.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
Arjan van de Ven <arjan@...ux.intel.com>,
Namhyung Kim <namhyung@...il.com>,
Yanmin Zhang <yanmin.zhang@...el.com>,
Wu Fengguang <fengguang.wu@...el.com>,
Mike Galbraith <efault@....de>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v3 1/8]Perf: Transform thread_map to linked list
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists