* [PATCH 1/4] perf tools: Use an accessor to read thread comm
2013-09-12 20:29 [PATCH 0/4] perf tools: New comm infrastructure Frederic Weisbecker
@ 2013-09-12 20:29 ` Frederic Weisbecker
2013-09-12 20:29 ` [PATCH 2/4] perf tools: Add time argument on comm setting Frederic Weisbecker
` (4 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Frederic Weisbecker @ 2013-09-12 20:29 UTC (permalink / raw)
To: LKML
Cc: Frederic Weisbecker, Jiri Olsa, David Ahern, Ingo Molnar,
Namhyung Kim, Peter Zijlstra, Arnaldo Carvalho de Melo,
Stephane Eranian
As the thread comm is going to be implemented by way of a more
complicated data structure than just a pointer to a string from the
thread struct, convert the readers of comm to use an accessor instead
of accessing it directly. The accessor will be later overriden to
support an enhanced comm implementation.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
---
tools/perf/builtin-kmem.c | 2 +-
tools/perf/builtin-lock.c | 2 +-
tools/perf/builtin-sched.c | 16 ++++++++--------
tools/perf/builtin-script.c | 6 +++---
tools/perf/builtin-trace.c | 2 +-
tools/perf/tests/hists_link.c | 2 +-
tools/perf/ui/browsers/hists.c | 10 +++++-----
tools/perf/util/event.c | 4 ++--
.../perf/util/scripting-engines/trace-event-perl.c | 2 +-
.../util/scripting-engines/trace-event-python.c | 4 ++--
tools/perf/util/sort.c | 10 +++++-----
tools/perf/util/thread.c | 7 ++++++-
tools/perf/util/thread.h | 1 +
13 files changed, 37 insertions(+), 31 deletions(-)
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index c2dff9c..707e860 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -314,7 +314,7 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
return -1;
}
- dump_printf(" ... thread: %s:%d\n", thread->comm, thread->tid);
+ dump_printf(" ... thread: %s:%d\n", thread__comm_curr(thread), thread->tid);
if (evsel->handler.func != NULL) {
tracepoint_handler f = evsel->handler.func;
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index ee33ba2..a7862ed 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -762,7 +762,7 @@ static void dump_threads(void)
while (node) {
st = container_of(node, struct thread_stat, rb);
t = perf_session__findnew(session, st->tid);
- pr_info("%10d: %s\n", st->tid, t->comm);
+ pr_info("%10d: %s\n", st->tid, thread__comm_curr(t));
node = rb_next(node);
};
}
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index d8c51b2..2699719 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -737,12 +737,12 @@ static int replay_fork_event(struct perf_sched *sched,
if (verbose) {
printf("fork event\n");
- printf("... parent: %s/%d\n", parent->comm, parent->tid);
- printf("... child: %s/%d\n", child->comm, child->tid);
+ printf("... parent: %s/%d\n", thread__comm_curr(parent), parent->tid);
+ printf("... child: %s/%d\n", thread__comm_curr(child), child->tid);
}
- register_pid(sched, parent->tid, parent->comm);
- register_pid(sched, child->tid, child->comm);
+ register_pid(sched, parent->tid, thread__comm_curr(parent));
+ register_pid(sched, child->tid, thread__comm_curr(child));
return 0;
}
@@ -1077,7 +1077,7 @@ static int latency_migrate_task_event(struct perf_sched *sched,
if (!atoms) {
if (thread_atoms_insert(sched, migrant))
return -1;
- register_pid(sched, migrant->tid, migrant->comm);
+ register_pid(sched, migrant->tid, thread__comm_curr(migrant));
atoms = thread_atoms_search(&sched->atom_root, migrant, &sched->cmp_pid);
if (!atoms) {
pr_err("migration-event: Internal tree error");
@@ -1111,13 +1111,13 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
/*
* Ignore idle threads:
*/
- if (!strcmp(work_list->thread->comm, "swapper"))
+ if (!strcmp(thread__comm_curr(work_list->thread), "swapper"))
return;
sched->all_runtime += work_list->total_runtime;
sched->all_count += work_list->nb_atoms;
- ret = printf(" %s:%d ", work_list->thread->comm, work_list->thread->tid);
+ ret = printf(" %s:%d ", thread__comm_curr(work_list->thread), work_list->thread->tid);
for (i = 0; i < 24 - ret; i++)
printf(" ");
@@ -1334,7 +1334,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,
printf(" %12.6f secs ", (double)timestamp/1e9);
if (new_shortname) {
printf("%s => %s:%d\n",
- sched_in->shortname, sched_in->comm, sched_in->tid);
+ sched_in->shortname, thread__comm_curr(sched_in), sched_in->tid);
} else {
printf("\n");
}
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 93a34ce..b98ddeb 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -290,11 +290,11 @@ static void print_sample_start(struct perf_sample *sample,
if (PRINT_FIELD(COMM)) {
if (latency_format)
- printf("%8.8s ", thread->comm);
+ printf("%8.8s ", thread__comm_curr(thread));
else if (PRINT_FIELD(IP) && symbol_conf.use_callchain)
- printf("%s ", thread->comm);
+ printf("%s ", thread__comm_curr(thread));
else
- printf("%16s ", thread->comm);
+ printf("%16s ", thread__comm_curr(thread));
}
if (PRINT_FIELD(PID) && PRINT_FIELD(TID))
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index f5aa637..d14ec25 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1125,7 +1125,7 @@ static size_t trace__fprintf_thread_summary(struct trace *trace, FILE *fp)
else if (ratio > 5.0)
color = PERF_COLOR_YELLOW;
- printed += color_fprintf(fp, color, "%20s", thread->comm);
+ printed += color_fprintf(fp, color, "%20s", thread__comm_curr(thread));
printed += fprintf(fp, " - %-5d :%11lu [", thread->tid, ttrace->nr_events);
printed += color_fprintf(fp, color, "%5.1f%%", ratio);
printed += fprintf(fp, " ] %10.3f ms\n", ttrace->runtime_ms);
diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
index 4228ffc..274be5d 100644
--- a/tools/perf/tests/hists_link.c
+++ b/tools/perf/tests/hists_link.c
@@ -419,7 +419,7 @@ static void print_hists(struct hists *hists)
he = rb_entry(node, struct hist_entry, rb_node_in);
pr_info("%2d: entry: %-8s [%-8s] %20s: period = %"PRIu64"\n",
- i, he->thread->comm, he->ms.map->dso->short_name,
+ i, thread__comm_curr(he->thread), he->ms.map->dso->short_name,
he->ms.sym->name, he->stat.period);
i++;
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7ef36c3..abb68e2 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1255,7 +1255,7 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size,
if (thread)
printed += scnprintf(bf + printed, size - printed,
", Thread: %s(%d)",
- (thread->comm_set ? thread->comm : ""),
+ (thread->comm_set ? thread__comm_curr(thread) : ""),
thread->tid);
if (dso)
printed += scnprintf(bf + printed, size - printed,
@@ -1578,7 +1578,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
if (thread != NULL &&
asprintf(&options[nr_options], "Zoom %s %s(%d) thread",
(browser->hists->thread_filter ? "out of" : "into"),
- (thread->comm_set ? thread->comm : ""),
+ (thread->comm_set ? thread__comm_curr(thread) : ""),
thread->tid) > 0)
zoom_thread = nr_options++;
@@ -1598,7 +1598,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
struct symbol *sym;
if (asprintf(&options[nr_options], "Run scripts for samples of thread [%s]",
- browser->he_selection->thread->comm) > 0)
+ thread__comm_curr(browser->he_selection->thread)) > 0)
scripts_comm = nr_options++;
sym = browser->he_selection->ms.sym;
@@ -1701,7 +1701,7 @@ zoom_out_thread:
sort_thread.elide = false;
} else {
ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"",
- thread->comm_set ? thread->comm : "",
+ thread->comm_set ? thread__comm_curr(thread) : "",
thread->tid);
browser->hists->thread_filter = thread;
sort_thread.elide = true;
@@ -1717,7 +1717,7 @@ do_scripts:
memset(script_opt, 0, 64);
if (choice == scripts_comm)
- sprintf(script_opt, " -c %s ", browser->he_selection->thread->comm);
+ sprintf(script_opt, " -c %s ", thread__comm_curr(browser->he_selection->thread));
if (choice == scripts_symbol)
sprintf(script_opt, " -S %s ", browser->he_selection->ms.sym->name);
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 8d51f21..c3a35ca 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -693,10 +693,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
return -1;
if (symbol_conf.comm_list &&
- !strlist__has_entry(symbol_conf.comm_list, thread->comm))
+ !strlist__has_entry(symbol_conf.comm_list, thread__comm_curr(thread)))
goto out_filtered;
- dump_printf(" ... thread: %s:%d\n", thread->comm, thread->tid);
+ dump_printf(" ... thread: %s:%d\n", thread__comm_curr(thread), thread->tid);
/*
* Have we already created the kernel maps for this machine?
*
diff --git a/tools/perf/util/scripting-engines/trace-event-perl.c b/tools/perf/util/scripting-engines/trace-event-perl.c
index a85e4ae..c4065ed 100644
--- a/tools/perf/util/scripting-engines/trace-event-perl.c
+++ b/tools/perf/util/scripting-engines/trace-event-perl.c
@@ -273,7 +273,7 @@ static void perl_process_tracepoint(union perf_event *perf_event __maybe_unused,
int cpu = sample->cpu;
void *data = sample->raw_data;
unsigned long long nsecs = sample->time;
- char *comm = thread->comm;
+ const char *comm = thread__comm_curr(thread);
dSP;
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index cc75a3c..422a78d 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -239,7 +239,7 @@ static void python_process_tracepoint(union perf_event *perf_event
int cpu = sample->cpu;
void *data = sample->raw_data;
unsigned long long nsecs = sample->time;
- char *comm = thread->comm;
+ char *comm = thread__comm_curr(thread);
t = PyTuple_New(MAX_FIELDS);
if (!t)
@@ -378,7 +378,7 @@ static void python_process_general_event(union perf_event *perf_event
PyDict_SetItemString(dict, "raw_buf", PyString_FromStringAndSize(
(const char *)sample->raw_data, sample->raw_size));
PyDict_SetItemString(dict, "comm",
- PyString_FromString(thread->comm));
+ PyString_FromString(thread__comm_curr(thread)));
if (al->map) {
PyDict_SetItemString(dict, "dso",
PyString_FromString(al->map->dso->name));
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 5f118a0..26d8f11 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -42,7 +42,7 @@ static int repsep_snprintf(char *bf, size_t size, const char *fmt, ...)
return n;
}
-static int64_t cmp_null(void *l, void *r)
+static int64_t cmp_null(const void *l, const void *r)
{
if (!l && !r)
return 0;
@@ -64,7 +64,7 @@ static int hist_entry__thread_snprintf(struct hist_entry *self, char *bf,
size_t size, unsigned int width)
{
return repsep_snprintf(bf, size, "%*s:%5d", width - 6,
- self->thread->comm ?: "", self->thread->tid);
+ thread__comm_curr(self->thread) ?: "", self->thread->tid);
}
struct sort_entry sort_thread = {
@@ -85,8 +85,8 @@ sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
static int64_t
sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
{
- char *comm_l = left->thread->comm;
- char *comm_r = right->thread->comm;
+ const char *comm_l = thread__comm_curr(left->thread);
+ const char *comm_r = thread__comm_curr(right->thread);
if (!comm_l || !comm_r)
return cmp_null(comm_l, comm_r);
@@ -97,7 +97,7 @@ sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
static int hist_entry__comm_snprintf(struct hist_entry *self, char *bf,
size_t size, unsigned int width)
{
- return repsep_snprintf(bf, size, "%*s", width, self->thread->comm);
+ return repsep_snprintf(bf, size, "%*s", width, thread__comm_curr(self->thread));
}
struct sort_entry sort_comm = {
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index e3d4a55..a139451 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -45,6 +45,11 @@ int thread__set_comm(struct thread *self, const char *comm)
return err;
}
+const char *thread__comm_curr(struct thread *self)
+{
+ return self->comm;
+}
+
int thread__comm_len(struct thread *self)
{
if (!self->comm_len) {
@@ -58,7 +63,7 @@ int thread__comm_len(struct thread *self)
size_t thread__fprintf(struct thread *thread, FILE *fp)
{
- return fprintf(fp, "Thread %d %s\n", thread->tid, thread->comm) +
+ return fprintf(fp, "Thread %d %s\n", thread->tid, thread__comm_curr(thread)) +
map_groups__fprintf(&thread->mg, verbose, fp);
}
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 4ebbb40..4eccee7 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -35,6 +35,7 @@ static inline void thread__exited(struct thread *thread)
int thread__set_comm(struct thread *self, const char *comm);
int thread__comm_len(struct thread *self);
+const char *thread__comm_curr(struct thread *self);
void thread__insert_map(struct thread *self, struct map *map);
int thread__fork(struct thread *self, struct thread *parent);
size_t thread__fprintf(struct thread *thread, FILE *fp);
--
1.7.5.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 2/4] perf tools: Add time argument on comm setting
2013-09-12 20:29 [PATCH 0/4] perf tools: New comm infrastructure Frederic Weisbecker
2013-09-12 20:29 ` [PATCH 1/4] perf tools: Use an accessor to read thread comm Frederic Weisbecker
@ 2013-09-12 20:29 ` Frederic Weisbecker
2013-09-12 20:29 ` [PATCH 3/4] perf tools: Add new comm infrastructure Frederic Weisbecker
` (3 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Frederic Weisbecker @ 2013-09-12 20:29 UTC (permalink / raw)
To: LKML
Cc: Frederic Weisbecker, Jiri Olsa, David Ahern, Ingo Molnar,
Namhyung Kim, Peter Zijlstra, Arnaldo Carvalho de Melo,
Stephane Eranian
This way we can later delimit a lifecycle for the comm and map
a hist to a precise comm:timeslice couple.
Comm and fork events that don't have PERF_SAMPLE_TIME samples
can only send 0 value as a timestamp and thus should overwrite any
previous comm on a given thread because there is no sensible way
to keep track of all the comms lifecycles in a thread without
time informations.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
---
tools/perf/builtin-top.c | 2 +-
tools/perf/builtin-trace.c | 12 ++++++------
tools/perf/tests/code-reading.c | 2 +-
tools/perf/tests/hists_link.c | 4 ++--
tools/perf/util/event.c | 24 ++++++++++++------------
tools/perf/util/machine.c | 34 +++++++++++++++++++---------------
tools/perf/util/machine.h | 18 ++++++++++++------
tools/perf/util/session.c | 2 +-
tools/perf/util/thread.c | 6 ++++--
tools/perf/util/thread.h | 4 ++--
10 files changed, 60 insertions(+), 48 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 2122141..ccf707d 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -856,7 +856,7 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
&sample, machine);
} else if (event->header.type < PERF_RECORD_MAX) {
hists__inc_nr_events(&evsel->hists, event->header.type);
- machine__process_event(machine, event);
+ machine__process_event(machine, event, &sample);
} else
++session->stats.nr_unknown_events;
}
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index d14ec25..a57063e 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -456,7 +456,7 @@ static size_t trace__fprintf_entry_head(struct trace *trace, struct thread *thre
}
static int trace__process_event(struct trace *trace, struct machine *machine,
- union perf_event *event)
+ union perf_event *event, struct perf_sample *sample)
{
int ret = 0;
@@ -464,9 +464,9 @@ static int trace__process_event(struct trace *trace, struct machine *machine,
case PERF_RECORD_LOST:
color_fprintf(trace->output, PERF_COLOR_RED,
"LOST %" PRIu64 " events!\n", event->lost.lost);
- ret = machine__process_lost_event(machine, event);
+ ret = machine__process_lost_event(machine, event, sample);
default:
- ret = machine__process_event(machine, event);
+ ret = machine__process_event(machine, event, sample);
break;
}
@@ -475,11 +475,11 @@ static int trace__process_event(struct trace *trace, struct machine *machine,
static int trace__tool_process(struct perf_tool *tool,
union perf_event *event,
- struct perf_sample *sample __maybe_unused,
+ struct perf_sample *sample,
struct machine *machine)
{
struct trace *trace = container_of(tool, struct trace, tool);
- return trace__process_event(trace, machine, event);
+ return trace__process_event(trace, machine, event, sample);
}
static int trace__symbols_init(struct trace *trace, struct perf_evlist *evlist)
@@ -977,7 +977,7 @@ again:
trace->base_time = sample.time;
if (type != PERF_RECORD_SAMPLE) {
- trace__process_event(trace, &trace->host, event);
+ trace__process_event(trace, &trace->host, event, &sample);
continue;
}
diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index 6fb781d..38d233a 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -276,7 +276,7 @@ static int process_event(struct machine *machine, struct perf_evlist *evlist,
return process_sample_event(machine, evlist, event, state);
if (event->header.type < PERF_RECORD_MAX)
- return machine__process_event(machine, event);
+ return machine__process_event(machine, event, NULL);
return 0;
}
diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
index 274be5d..a97063d 100644
--- a/tools/perf/tests/hists_link.c
+++ b/tools/perf/tests/hists_link.c
@@ -93,7 +93,7 @@ static struct machine *setup_fake_machine(struct machines *machines)
if (thread == NULL)
goto out;
- thread__set_comm(thread, fake_threads[i].comm);
+ thread__set_comm(thread, fake_threads[i].comm, 0);
}
for (i = 0; i < ARRAY_SIZE(fake_mmap_info); i++) {
@@ -110,7 +110,7 @@ static struct machine *setup_fake_machine(struct machines *machines)
strcpy(fake_mmap_event.mmap.filename,
fake_mmap_info[i].filename);
- machine__process_mmap_event(machine, &fake_mmap_event);
+ machine__process_mmap_event(machine, &fake_mmap_event, NULL);
}
for (i = 0; i < ARRAY_SIZE(fake_symbols); i++) {
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index c3a35ca..5c9508a 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -506,18 +506,18 @@ size_t perf_event__fprintf_comm(union perf_event *event, FILE *fp)
int perf_event__process_comm(struct perf_tool *tool __maybe_unused,
union perf_event *event,
- struct perf_sample *sample __maybe_unused,
+ struct perf_sample *sample,
struct machine *machine)
{
- return machine__process_comm_event(machine, event);
+ return machine__process_comm_event(machine, event, sample);
}
int perf_event__process_lost(struct perf_tool *tool __maybe_unused,
union perf_event *event,
- struct perf_sample *sample __maybe_unused,
+ struct perf_sample *sample,
struct machine *machine)
{
- return machine__process_lost_event(machine, event);
+ return machine__process_lost_event(machine, event, sample);
}
size_t perf_event__fprintf_mmap(union perf_event *event, FILE *fp)
@@ -529,10 +529,10 @@ size_t perf_event__fprintf_mmap(union perf_event *event, FILE *fp)
int perf_event__process_mmap(struct perf_tool *tool __maybe_unused,
union perf_event *event,
- struct perf_sample *sample __maybe_unused,
+ struct perf_sample *sample,
struct machine *machine)
{
- return machine__process_mmap_event(machine, event);
+ return machine__process_mmap_event(machine, event, sample);
}
size_t perf_event__fprintf_task(union perf_event *event, FILE *fp)
@@ -544,18 +544,18 @@ size_t perf_event__fprintf_task(union perf_event *event, FILE *fp)
int perf_event__process_fork(struct perf_tool *tool __maybe_unused,
union perf_event *event,
- struct perf_sample *sample __maybe_unused,
+ struct perf_sample *sample,
struct machine *machine)
{
- return machine__process_fork_event(machine, event);
+ return machine__process_fork_event(machine, event, sample);
}
int perf_event__process_exit(struct perf_tool *tool __maybe_unused,
union perf_event *event,
- struct perf_sample *sample __maybe_unused,
+ struct perf_sample *sample,
struct machine *machine)
{
- return machine__process_exit_event(machine, event);
+ return machine__process_exit_event(machine, event, sample);
}
size_t perf_event__fprintf(union perf_event *event, FILE *fp)
@@ -583,10 +583,10 @@ size_t perf_event__fprintf(union perf_event *event, FILE *fp)
int perf_event__process(struct perf_tool *tool __maybe_unused,
union perf_event *event,
- struct perf_sample *sample __maybe_unused,
+ struct perf_sample *sample,
struct machine *machine)
{
- return machine__process_event(machine, event);
+ return machine__process_event(machine, event, sample);
}
void thread__find_addr_map(struct thread *self,
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 1dca61f..c862686 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -40,7 +40,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
return -ENOMEM;
snprintf(comm, sizeof(comm), "[guest/%d]", pid);
- thread__set_comm(thread, comm);
+ thread__set_comm(thread, comm, 0);
}
return 0;
@@ -314,7 +314,8 @@ struct thread *machine__find_thread(struct machine *machine, pid_t tid)
return __machine__findnew_thread(machine, 0, tid, false);
}
-int machine__process_comm_event(struct machine *machine, union perf_event *event)
+int machine__process_comm_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample)
{
struct thread *thread = machine__findnew_thread(machine,
event->comm.pid,
@@ -323,7 +324,7 @@ int machine__process_comm_event(struct machine *machine, union perf_event *event
if (dump_trace)
perf_event__fprintf_comm(event, stdout);
- if (thread == NULL || thread__set_comm(thread, event->comm.comm)) {
+ if (thread == NULL || thread__set_comm(thread, event->comm.comm, sample->time)) {
dump_printf("problem processing PERF_RECORD_COMM, skipping event.\n");
return -1;
}
@@ -332,7 +333,7 @@ int machine__process_comm_event(struct machine *machine, union perf_event *event
}
int machine__process_lost_event(struct machine *machine __maybe_unused,
- union perf_event *event)
+ union perf_event *event, struct perf_sample *sample __maybe_unused)
{
dump_printf(": id:%" PRIu64 ": lost:%" PRIu64 "\n",
event->lost.id, event->lost.lost);
@@ -997,7 +998,8 @@ out_problem:
return -1;
}
-int machine__process_mmap_event(struct machine *machine, union perf_event *event)
+int machine__process_mmap_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample __maybe_unused)
{
u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
struct thread *thread;
@@ -1053,7 +1055,8 @@ static void machine__remove_thread(struct machine *machine, struct thread *th)
list_add_tail(&th->node, &machine->dead_threads);
}
-int machine__process_fork_event(struct machine *machine, union perf_event *event)
+int machine__process_fork_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample)
{
struct thread *thread = machine__find_thread(machine, event->fork.tid);
struct thread *parent = machine__findnew_thread(machine,
@@ -1070,7 +1073,7 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
perf_event__fprintf_task(event, stdout);
if (thread == NULL || parent == NULL ||
- thread__fork(thread, parent) < 0) {
+ thread__fork(thread, parent, sample->time) < 0) {
dump_printf("problem processing PERF_RECORD_FORK, skipping event.\n");
return -1;
}
@@ -1078,8 +1081,8 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
return 0;
}
-int machine__process_exit_event(struct machine *machine __maybe_unused,
- union perf_event *event)
+int machine__process_exit_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample __maybe_unused)
{
struct thread *thread = machine__find_thread(machine, event->fork.tid);
@@ -1092,21 +1095,22 @@ int machine__process_exit_event(struct machine *machine __maybe_unused,
return 0;
}
-int machine__process_event(struct machine *machine, union perf_event *event)
+int machine__process_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample)
{
int ret;
switch (event->header.type) {
case PERF_RECORD_COMM:
- ret = machine__process_comm_event(machine, event); break;
+ ret = machine__process_comm_event(machine, event, sample); break;
case PERF_RECORD_MMAP:
- ret = machine__process_mmap_event(machine, event); break;
+ ret = machine__process_mmap_event(machine, event, sample); break;
case PERF_RECORD_FORK:
- ret = machine__process_fork_event(machine, event); break;
+ ret = machine__process_fork_event(machine, event, sample); break;
case PERF_RECORD_EXIT:
- ret = machine__process_exit_event(machine, event); break;
+ ret = machine__process_exit_event(machine, event, sample); break;
case PERF_RECORD_LOST:
- ret = machine__process_lost_event(machine, event); break;
+ ret = machine__process_lost_event(machine, event, sample); break;
default:
ret = -1;
break;
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index 0df925b..fda2839 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -40,12 +40,18 @@ struct map *machine__kernel_map(struct machine *machine, enum map_type type)
struct thread *machine__find_thread(struct machine *machine, pid_t tid);
-int machine__process_comm_event(struct machine *machine, union perf_event *event);
-int machine__process_exit_event(struct machine *machine, union perf_event *event);
-int machine__process_fork_event(struct machine *machine, union perf_event *event);
-int machine__process_lost_event(struct machine *machine, union perf_event *event);
-int machine__process_mmap_event(struct machine *machine, union perf_event *event);
-int machine__process_event(struct machine *machine, union perf_event *event);
+int machine__process_comm_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample);
+int machine__process_exit_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample);
+int machine__process_fork_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample);
+int machine__process_lost_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample);
+int machine__process_mmap_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample);
+int machine__process_event(struct machine *machine, union perf_event *event,
+ struct perf_sample *sample);
typedef void (*machine__process_t)(struct machine *machine, void *data);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 476caa1..8ef2d6d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1089,7 +1089,7 @@ static struct thread *perf_session__register_idle_thread(struct perf_session *se
{
struct thread *thread = perf_session__findnew(self, 0);
- if (thread == NULL || thread__set_comm(thread, "swapper")) {
+ if (thread == NULL || thread__set_comm(thread, "swapper", 0)) {
pr_err("problem inserting idle task.\n");
thread = NULL;
}
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index a139451..1466baf 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -31,7 +31,8 @@ void thread__delete(struct thread *self)
free(self);
}
-int thread__set_comm(struct thread *self, const char *comm)
+int thread__set_comm(struct thread *self, const char *comm,
+ u64 timestamp __maybe_unused)
{
int err;
@@ -73,7 +74,8 @@ void thread__insert_map(struct thread *self, struct map *map)
map_groups__insert(&self->mg, map);
}
-int thread__fork(struct thread *self, struct thread *parent)
+int thread__fork(struct thread *self, struct thread *parent,
+ u64 timestamp __maybe_unused)
{
int i;
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 4eccee7..fda69d9 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -33,11 +33,11 @@ static inline void thread__exited(struct thread *thread)
thread->dead = true;
}
-int thread__set_comm(struct thread *self, const char *comm);
+int thread__set_comm(struct thread *self, const char *comm, u64 timestamp);
int thread__comm_len(struct thread *self);
const char *thread__comm_curr(struct thread *self);
void thread__insert_map(struct thread *self, struct map *map);
-int thread__fork(struct thread *self, struct thread *parent);
+int thread__fork(struct thread *self, struct thread *parent, u64 timestamp);
size_t thread__fprintf(struct thread *thread, FILE *fp);
static inline struct map *thread__find_map(struct thread *self,
--
1.7.5.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 3/4] perf tools: Add new comm infrastructure
2013-09-12 20:29 [PATCH 0/4] perf tools: New comm infrastructure Frederic Weisbecker
2013-09-12 20:29 ` [PATCH 1/4] perf tools: Use an accessor to read thread comm Frederic Weisbecker
2013-09-12 20:29 ` [PATCH 2/4] perf tools: Add time argument on comm setting Frederic Weisbecker
@ 2013-09-12 20:29 ` Frederic Weisbecker
2013-09-12 20:29 ` [PATCH 4/4] perf tools: Compare hists comm by addresses Frederic Weisbecker
` (2 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Frederic Weisbecker @ 2013-09-12 20:29 UTC (permalink / raw)
To: LKML
Cc: Frederic Weisbecker, Jiri Olsa, David Ahern, Ingo Molnar,
Namhyung Kim, Peter Zijlstra, Arnaldo Carvalho de Melo,
Stephane Eranian
This new comm infrastructure provides two features:
1) It keeps track of all comms lifecycle for a given thread. This
way we can associate a timeframe to any thread comm, as long as
PERF_SAMPLE_TIME samples are joined to comm and fork events.
As a result we should have more precise comm sorted hists with
seperated entries for pre and post exec time after a fork.
2) It also makes sure that a given comm string is not duplicated
but rather shared among the threads that refer to it. This way the
threads comm can be compared against pointer values from the sort
infrastructure.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
---
tools/perf/Makefile | 2 +
tools/perf/util/comm.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/comm.h | 20 +++++++++
tools/perf/util/thread.c | 92 +++++++++++++++++++++++++++++----------
tools/perf/util/thread.h | 3 +-
5 files changed, 199 insertions(+), 25 deletions(-)
create mode 100644 tools/perf/util/comm.c
create mode 100644 tools/perf/util/comm.h
diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 3a0ff7f..36e6958 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -270,6 +270,7 @@ LIB_H += util/color.h
LIB_H += util/values.h
LIB_H += util/sort.h
LIB_H += util/hist.h
+LIB_H += util/comm.h
LIB_H += util/thread.h
LIB_H += util/thread_map.h
LIB_H += util/trace-event.h
@@ -337,6 +338,7 @@ LIB_OBJS += $(OUTPUT)util/machine.o
LIB_OBJS += $(OUTPUT)util/map.o
LIB_OBJS += $(OUTPUT)util/pstack.o
LIB_OBJS += $(OUTPUT)util/session.o
+LIB_OBJS += $(OUTPUT)util/comm.o
LIB_OBJS += $(OUTPUT)util/thread.o
LIB_OBJS += $(OUTPUT)util/thread_map.o
LIB_OBJS += $(OUTPUT)util/trace-event-parse.o
diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
new file mode 100644
index 0000000..87f4a10
--- /dev/null
+++ b/tools/perf/util/comm.c
@@ -0,0 +1,107 @@
+#include "comm.h"
+#include "util.h"
+#include <stdlib.h>
+#include <stdio.h>
+
+struct comm_str {
+ char *str;
+ struct rb_node rb_node;
+ int ref;
+};
+
+/* Should perhaps be moved to struct machine */
+static struct rb_root comm_str_root;
+
+static void comm_str_get(struct comm_str *cs)
+{
+ cs->ref++;
+}
+
+static void comm_str_put(struct comm_str *cs)
+{
+ if (!--cs->ref) {
+ rb_erase(&cs->rb_node, &comm_str_root);
+ free(cs->str);
+ free(cs);
+ }
+}
+
+static struct comm_str *comm_str_alloc(const char *str)
+{
+ struct comm_str *cs;
+
+ cs = zalloc(sizeof(*cs));
+ if (!cs)
+ return NULL;
+
+ cs->str = strdup(str);
+ if (!cs->str) {
+ free(cs);
+ return NULL;
+ }
+
+ return cs;
+}
+
+static struct comm_str *comm_str_findnew(const char *str, struct rb_root *root)
+{
+ struct rb_node **p = &root->rb_node;
+ struct rb_node *parent = NULL;
+ struct comm_str *iter, *new;
+ int cmp;
+
+ while (*p != NULL) {
+ parent = *p;
+ iter = rb_entry(parent, struct comm_str, rb_node);
+
+ cmp = strcmp(str, iter->str);
+ if (!cmp)
+ return iter;
+
+ if (cmp < 0)
+ p = &(*p)->rb_left;
+ else
+ p = &(*p)->rb_right;
+ }
+
+ new = comm_str_alloc(str);
+ if (!new)
+ return NULL;
+
+ rb_link_node(&new->rb_node, parent, p);
+ rb_insert_color(&new->rb_node, root);
+
+ return new;
+}
+
+struct comm *comm__new(const char *str, u64 timestamp)
+{
+ struct comm *self;
+
+ self = zalloc(sizeof(*self));
+ if (!self)
+ return NULL;
+
+ self->start = timestamp;
+
+ self->comm_str = comm_str_findnew(str, &comm_str_root);
+ if (!self->comm_str) {
+ free(self);
+ return NULL;
+ }
+
+ comm_str_get(self->comm_str);
+
+ return self;
+}
+
+void comm__free(struct comm *self)
+{
+ comm_str_put(self->comm_str);
+ free(self);
+}
+
+const char *comm__str(struct comm *self)
+{
+ return self->comm_str->str;
+}
diff --git a/tools/perf/util/comm.h b/tools/perf/util/comm.h
new file mode 100644
index 0000000..2e47fb7
--- /dev/null
+++ b/tools/perf/util/comm.h
@@ -0,0 +1,20 @@
+#ifndef __PERF_COMM_H
+#define __PERF_COMM_H
+
+#include "../perf.h"
+#include <linux/rbtree.h>
+#include <linux/list.h>
+
+struct comm_str;
+
+struct comm {
+ struct comm_str *comm_str;
+ u64 start;
+ struct list_head list;
+};
+
+void comm__free(struct comm *self);
+struct comm *comm__new(const char *str, u64 timestamp);
+const char *comm__str(struct comm *self);
+
+#endif /* __PERF_COMM_H */
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 1466baf..63c4b78 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -6,9 +6,12 @@
#include "thread.h"
#include "util.h"
#include "debug.h"
+#include "comm.h"
struct thread *thread__new(pid_t pid, pid_t tid)
{
+ char *comm_str;
+ struct comm *comm;
struct thread *self = zalloc(sizeof(*self));
if (self != NULL) {
@@ -16,47 +19,88 @@ struct thread *thread__new(pid_t pid, pid_t tid)
self->pid_ = pid;
self->tid = tid;
self->ppid = -1;
- self->comm = malloc(32);
- if (self->comm)
- snprintf(self->comm, 32, ":%d", self->tid);
+ INIT_LIST_HEAD(&self->comm_list);
+
+ comm_str = malloc(32);
+ if (!comm_str)
+ goto err_thread;
+
+ snprintf(comm_str, 32, ":%d", tid);
+ comm = comm__new(comm_str, 0);
+ free(comm_str);
+ if (!comm)
+ goto err_thread;
+
+ list_add(&comm->list, &self->comm_list);
}
return self;
+
+err_thread:
+ free(self);
+ return NULL;
}
void thread__delete(struct thread *self)
{
+ struct comm *comm, *tmp;
+
map_groups__exit(&self->mg);
- free(self->comm);
+ list_for_each_entry_safe(comm, tmp, &self->comm_list, list) {
+ list_del(&comm->list);
+ comm__free(comm);
+ }
+
free(self);
}
-int thread__set_comm(struct thread *self, const char *comm,
- u64 timestamp __maybe_unused)
+static struct comm *curr_comm(struct thread *self)
{
- int err;
+ if (list_empty(&self->comm_list))
+ return NULL;
- if (self->comm)
- free(self->comm);
- self->comm = strdup(comm);
- err = self->comm == NULL ? -ENOMEM : 0;
- if (!err) {
- self->comm_set = true;
+ return list_first_entry(&self->comm_list, struct comm, list);
+}
+
+/* CHECKME: time should always be 0 if event aren't ordered */
+int thread__set_comm(struct thread *self, const char *str, u64 timestamp)
+{
+ struct comm *new, *curr = curr_comm(self);
+
+ /* Override latest entry if it had no specific time coverage */
+ if (!curr->start) {
+ list_del(&curr->list);
+ comm__free(curr);
}
- return err;
+
+ new = comm__new(str, timestamp);
+ if (!new)
+ return -ENOMEM;
+
+ list_add(&new->list, &self->comm_list);
+ self->comm_set = true;
+
+ return 0;
}
const char *thread__comm_curr(struct thread *self)
{
- return self->comm;
+ struct comm *comm = curr_comm(self);
+
+ if (!comm)
+ return NULL;
+
+ return comm__str(comm);
}
+/* CHECKME: it should probably better return the max comm len from its comm list */
int thread__comm_len(struct thread *self)
{
if (!self->comm_len) {
- if (!self->comm)
+ const char *comm = thread__comm_curr(self);
+ if (!comm)
return 0;
- self->comm_len = strlen(self->comm);
+ self->comm_len = strlen(comm);
}
return self->comm_len;
@@ -74,17 +118,17 @@ void thread__insert_map(struct thread *self, struct map *map)
map_groups__insert(&self->mg, map);
}
-int thread__fork(struct thread *self, struct thread *parent,
- u64 timestamp __maybe_unused)
+int thread__fork(struct thread *self, struct thread *parent, u64 timestamp)
{
- int i;
+ int i, err;
if (parent->comm_set) {
- if (self->comm)
- free(self->comm);
- self->comm = strdup(parent->comm);
- if (!self->comm)
+ const char *comm = thread__comm_curr(parent);
+ if (!comm)
return -ENOMEM;
+ err = thread__set_comm(self, comm, timestamp);
+ if (!err)
+ return err;
self->comm_set = true;
}
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index fda69d9..f55416b 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -2,6 +2,7 @@
#define __PERF_THREAD_H
#include <linux/rbtree.h>
+#include <linux/list.h>
#include <unistd.h>
#include <sys/types.h>
#include "symbol.h"
@@ -18,7 +19,7 @@ struct thread {
char shortname[3];
bool comm_set;
bool dead; /* if set thread has exited */
- char *comm;
+ struct list_head comm_list;
int comm_len;
void *priv;
--
1.7.5.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 4/4] perf tools: Compare hists comm by addresses
2013-09-12 20:29 [PATCH 0/4] perf tools: New comm infrastructure Frederic Weisbecker
` (2 preceding siblings ...)
2013-09-12 20:29 ` [PATCH 3/4] perf tools: Add new comm infrastructure Frederic Weisbecker
@ 2013-09-12 20:29 ` Frederic Weisbecker
2013-09-13 8:07 ` Namhyung Kim
2013-09-12 20:36 ` [PATCH 0/4] perf tools: New comm infrastructure Ingo Molnar
2013-09-13 6:32 ` Namhyung Kim
5 siblings, 1 reply; 20+ messages in thread
From: Frederic Weisbecker @ 2013-09-12 20:29 UTC (permalink / raw)
To: LKML
Cc: Frederic Weisbecker, Jiri Olsa, David Ahern, Ingo Molnar,
Namhyung Kim, Peter Zijlstra, Arnaldo Carvalho de Melo,
Stephane Eranian
Now that comm strings are allocated only once and refcounted to be shared
among threads, these can now be safely compared by addresses. This
should remove most hists collapses on post processing.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
---
tools/perf/util/sort.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 26d8f11..3b307e7 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -79,7 +79,8 @@ struct sort_entry sort_thread = {
static int64_t
sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
{
- return right->thread->tid - left->thread->tid;
+ /* Compare the addr that should be unique among comm */
+ return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
}
static int64_t
--
1.7.5.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/4] perf tools: Compare hists comm by addresses
2013-09-12 20:29 ` [PATCH 4/4] perf tools: Compare hists comm by addresses Frederic Weisbecker
@ 2013-09-13 8:07 ` Namhyung Kim
2013-09-13 13:59 ` Frederic Weisbecker
0 siblings, 1 reply; 20+ messages in thread
From: Namhyung Kim @ 2013-09-13 8:07 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Peter Zijlstra,
Arnaldo Carvalho de Melo, Stephane Eranian
Hi,
On Thu, 12 Sep 2013 22:29:43 +0200, Frederic Weisbecker wrote:
> Now that comm strings are allocated only once and refcounted to be shared
> among threads, these can now be safely compared by addresses. This
> should remove most hists collapses on post processing.
[SNIP]
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 26d8f11..3b307e7 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -79,7 +79,8 @@ struct sort_entry sort_thread = {
> static int64_t
> sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
> {
> - return right->thread->tid - left->thread->tid;
> + /* Compare the addr that should be unique among comm */
> + return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
I don't think this is enough. A hist entry should reference the comm
itself at that time. Saving thread can lead to referencing different
comm between insert and collapse time if thread changed the comm in the
meanwhile, right?
What about something like below:
>From 431fd9bfa844ddfe28ab1390959bc7de28804a9c Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung.kim@lge.com>
Date: Fri, 13 Sep 2013 16:28:57 +0900
Subject: [PATCH] perf tools: Get current comm instead of last one
At insert time, a hist entry should reference comm at the time
otherwise it can get a different comm later.
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/hist.c | 3 +++
tools/perf/util/sort.c | 9 +++++----
tools/perf/util/sort.h | 1 +
tools/perf/util/thread.c | 10 ++--------
tools/perf/util/thread.h | 2 ++
5 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1f5767f97dce..1fe90bd9dcb7 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -412,6 +412,7 @@ struct hist_entry *__hists__add_mem_entry(struct hists *self,
{
struct hist_entry entry = {
.thread = al->thread,
+ .comm = curr_comm(al->thread),
.ms = {
.map = al->map,
.sym = al->sym,
@@ -442,6 +443,7 @@ struct hist_entry *__hists__add_branch_entry(struct hists *self,
{
struct hist_entry entry = {
.thread = al->thread,
+ .comm = curr_comm(al->thread),
.ms = {
.map = bi->to.map,
.sym = bi->to.sym,
@@ -471,6 +473,7 @@ struct hist_entry *__hists__add_entry(struct hists *self,
{
struct hist_entry entry = {
.thread = al->thread,
+ .comm = curr_comm(al->thread),
.ms = {
.map = al->map,
.sym = al->sym,
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 3b307e740d6e..840481859e4b 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1,5 +1,6 @@
#include "sort.h"
#include "hist.h"
+#include "comm.h"
#include "symbol.h"
regex_t parent_regex;
@@ -80,14 +81,14 @@ static int64_t
sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
{
/* Compare the addr that should be unique among comm */
- return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
+ return comm__str(right->comm) - comm__str(left->comm);
}
static int64_t
sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
{
- const char *comm_l = thread__comm_curr(left->thread);
- const char *comm_r = thread__comm_curr(right->thread);
+ const char *comm_l = comm__str(left->comm);
+ const char *comm_r = comm__str(right->comm);
if (!comm_l || !comm_r)
return cmp_null(comm_l, comm_r);
@@ -98,7 +99,7 @@ sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
static int hist_entry__comm_snprintf(struct hist_entry *self, char *bf,
size_t size, unsigned int width)
{
- return repsep_snprintf(bf, size, "%*s", width, thread__comm_curr(self->thread));
+ return repsep_snprintf(bf, size, "%*s", width, comm__str(self->comm));
}
struct sort_entry sort_comm = {
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 4e80dbd271e7..4d0153f83e3c 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -84,6 +84,7 @@ struct hist_entry {
struct he_stat stat;
struct map_symbol ms;
struct thread *thread;
+ struct comm *comm;
u64 ip;
s32 cpu;
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index d3ca133329b2..e7c7fe6694e8 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -54,7 +54,7 @@ void thread__delete(struct thread *self)
free(self);
}
-static struct comm *curr_comm(const struct thread *self)
+struct comm *curr_comm(const struct thread *self)
{
if (list_empty(&self->comm_list))
return NULL;
@@ -65,13 +65,7 @@ static struct comm *curr_comm(const struct thread *self)
/* CHECKME: time should always be 0 if event aren't ordered */
int thread__set_comm(struct thread *self, const char *str, u64 timestamp)
{
- struct comm *new, *curr = curr_comm(self);
-
- /* Override latest entry if it had no specific time coverage */
- if (!curr->start) {
- list_del(&curr->list);
- comm__free(curr);
- }
+ struct comm *new;
new = comm__new(str, timestamp);
if (!new)
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 1d9378abb515..cf8e31d0028b 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -26,6 +26,7 @@ struct thread {
};
struct machine;
+struct comm;
struct thread *thread__new(pid_t pid, pid_t tid);
void thread__delete(struct thread *self);
@@ -36,6 +37,7 @@ static inline void thread__exited(struct thread *thread)
int thread__set_comm(struct thread *self, const char *comm, u64 timestamp);
int thread__comm_len(struct thread *self);
+struct comm *curr_comm(const struct thread *self);
const char *thread__comm_curr(const struct thread *self);
void thread__insert_map(struct thread *self, struct map *map);
int thread__fork(struct thread *self, struct thread *parent, u64 timestamp);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/4] perf tools: Compare hists comm by addresses
2013-09-13 8:07 ` Namhyung Kim
@ 2013-09-13 13:59 ` Frederic Weisbecker
2013-09-17 1:46 ` Namhyung Kim
0 siblings, 1 reply; 20+ messages in thread
From: Frederic Weisbecker @ 2013-09-13 13:59 UTC (permalink / raw)
To: Namhyung Kim
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Peter Zijlstra,
Arnaldo Carvalho de Melo, Stephane Eranian
On Fri, Sep 13, 2013 at 05:07:06PM +0900, Namhyung Kim wrote:
> Hi,
>
> On Thu, 12 Sep 2013 22:29:43 +0200, Frederic Weisbecker wrote:
> > Now that comm strings are allocated only once and refcounted to be shared
> > among threads, these can now be safely compared by addresses. This
> > should remove most hists collapses on post processing.
>
> [SNIP]
>
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index 26d8f11..3b307e7 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -79,7 +79,8 @@ struct sort_entry sort_thread = {
> > static int64_t
> > sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
> > {
> > - return right->thread->tid - left->thread->tid;
> > + /* Compare the addr that should be unique among comm */
> > + return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
>
> I don't think this is enough. A hist entry should reference the comm
> itself at that time. Saving thread can lead to referencing different
> comm between insert and collapse time if thread changed the comm in the
> meanwhile, right?
Exactly! That's indeed what is missing in this patchset. The comm supports timed comm
but not yet the hists.
>
> What about something like below:
>
>
> From 431fd9bfa844ddfe28ab1390959bc7de28804a9c Mon Sep 17 00:00:00 2001
> From: Namhyung Kim <namhyung.kim@lge.com>
> Date: Fri, 13 Sep 2013 16:28:57 +0900
> Subject: [PATCH] perf tools: Get current comm instead of last one
>
> At insert time, a hist entry should reference comm at the time
> otherwise it can get a different comm later.
>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/util/hist.c | 3 +++
> tools/perf/util/sort.c | 9 +++++----
> tools/perf/util/sort.h | 1 +
> tools/perf/util/thread.c | 10 ++--------
> tools/perf/util/thread.h | 2 ++
> 5 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 1f5767f97dce..1fe90bd9dcb7 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -412,6 +412,7 @@ struct hist_entry *__hists__add_mem_entry(struct hists *self,
> {
> struct hist_entry entry = {
> .thread = al->thread,
> + .comm = curr_comm(al->thread),
> .ms = {
> .map = al->map,
> .sym = al->sym,
> @@ -442,6 +443,7 @@ struct hist_entry *__hists__add_branch_entry(struct hists *self,
> {
> struct hist_entry entry = {
> .thread = al->thread,
> + .comm = curr_comm(al->thread),
> .ms = {
> .map = bi->to.map,
> .sym = bi->to.sym,
> @@ -471,6 +473,7 @@ struct hist_entry *__hists__add_entry(struct hists *self,
> {
> struct hist_entry entry = {
> .thread = al->thread,
> + .comm = curr_comm(al->thread),
> .ms = {
> .map = al->map,
> .sym = al->sym,
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 3b307e740d6e..840481859e4b 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1,5 +1,6 @@
> #include "sort.h"
> #include "hist.h"
> +#include "comm.h"
> #include "symbol.h"
>
> regex_t parent_regex;
> @@ -80,14 +81,14 @@ static int64_t
> sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
> {
> /* Compare the addr that should be unique among comm */
> - return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
> + return comm__str(right->comm) - comm__str(left->comm);
If comm and fork events don't have a timestamp, or they aren't time ordered, we should
override the comm entry of a thread and forget the previous one.
So perhaps what we should do instead is to compare "struct comm" addresses directly.
But it means that on thread__set_comm(), if we override the old entry due to missing or
unordered timestamps (in which case we need to force them to be 0), we shouldn't reallocate
a new struct comm, nor should we keep the old one and queue a new. Instead we need to override
list_first_entry(thread->comm)->comm_str pointer only to make it point to a new struct comm_str.
> }
>
> static int64_t
> sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
> {
> - const char *comm_l = thread__comm_curr(left->thread);
> - const char *comm_r = thread__comm_curr(right->thread);
> + const char *comm_l = comm__str(left->comm);
> + const char *comm_r = comm__str(right->comm);
>
> if (!comm_l || !comm_r)
> return cmp_null(comm_l, comm_r);
> @@ -98,7 +99,7 @@ sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
> static int hist_entry__comm_snprintf(struct hist_entry *self, char *bf,
> size_t size, unsigned int width)
> {
> - return repsep_snprintf(bf, size, "%*s", width, thread__comm_curr(self->thread));
> + return repsep_snprintf(bf, size, "%*s", width, comm__str(self->comm));
> }
>
> struct sort_entry sort_comm = {
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 4e80dbd271e7..4d0153f83e3c 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -84,6 +84,7 @@ struct hist_entry {
> struct he_stat stat;
> struct map_symbol ms;
> struct thread *thread;
> + struct comm *comm;
> u64 ip;
> s32 cpu;
>
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index d3ca133329b2..e7c7fe6694e8 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -54,7 +54,7 @@ void thread__delete(struct thread *self)
> free(self);
> }
>
> -static struct comm *curr_comm(const struct thread *self)
> +struct comm *curr_comm(const struct thread *self)
> {
> if (list_empty(&self->comm_list))
> return NULL;
> @@ -65,13 +65,7 @@ static struct comm *curr_comm(const struct thread *self)
> /* CHECKME: time should always be 0 if event aren't ordered */
> int thread__set_comm(struct thread *self, const char *str, u64 timestamp)
> {
> - struct comm *new, *curr = curr_comm(self);
> -
> - /* Override latest entry if it had no specific time coverage */
> - if (!curr->start) {
> - list_del(&curr->list);
> - comm__free(curr);
> - }
We must still remove the old entry if timestamps aren't there or aren't ordered.
Just we need to keep the old "struct comm" and replace the comm_str.
Thanks.
> + struct comm *new;
>
> new = comm__new(str, timestamp);
> if (!new)
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 1d9378abb515..cf8e31d0028b 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -26,6 +26,7 @@ struct thread {
> };
>
> struct machine;
> +struct comm;
>
> struct thread *thread__new(pid_t pid, pid_t tid);
> void thread__delete(struct thread *self);
> @@ -36,6 +37,7 @@ static inline void thread__exited(struct thread *thread)
>
> int thread__set_comm(struct thread *self, const char *comm, u64 timestamp);
> int thread__comm_len(struct thread *self);
> +struct comm *curr_comm(const struct thread *self);
> const char *thread__comm_curr(const struct thread *self);
> void thread__insert_map(struct thread *self, struct map *map);
> int thread__fork(struct thread *self, struct thread *parent, u64 timestamp);
> --
> 1.7.11.7
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/4] perf tools: Compare hists comm by addresses
2013-09-13 13:59 ` Frederic Weisbecker
@ 2013-09-17 1:46 ` Namhyung Kim
0 siblings, 0 replies; 20+ messages in thread
From: Namhyung Kim @ 2013-09-17 1:46 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Peter Zijlstra,
Arnaldo Carvalho de Melo, Stephane Eranian
Hi Frederic,
On Fri, 13 Sep 2013 15:59:49 +0200, Frederic Weisbecker wrote:
> On Fri, Sep 13, 2013 at 05:07:06PM +0900, Namhyung Kim wrote:
[SNIP]
>> --- a/tools/perf/util/sort.c
>> +++ b/tools/perf/util/sort.c
>> @@ -1,5 +1,6 @@
>> #include "sort.h"
>> #include "hist.h"
>> +#include "comm.h"
>> #include "symbol.h"
>>
>> regex_t parent_regex;
>> @@ -80,14 +81,14 @@ static int64_t
>> sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
>> {
>> /* Compare the addr that should be unique among comm */
>> - return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
>> + return comm__str(right->comm) - comm__str(left->comm);
>
>
> If comm and fork events don't have a timestamp, or they aren't time ordered, we should
> override the comm entry of a thread and forget the previous one.
>
> So perhaps what we should do instead is to compare "struct comm" addresses directly.
> But it means that on thread__set_comm(), if we override the old entry due to missing or
> unordered timestamps (in which case we need to force them to be 0), we shouldn't reallocate
> a new struct comm, nor should we keep the old one and queue a new. Instead we need to override
> list_first_entry(thread->comm)->comm_str pointer only to make it point to a new struct comm_str.
Okay. Here's a revised patch:
>From 599221323f8fc0fd3327190900fece6c2aaa7309 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung.kim@lge.com>
Date: Fri, 13 Sep 2013 16:28:57 +0900
Subject: [PATCH] perf tools: Get current comm instead of last one
At insert time, a hist entry should reference comm at the time
otherwise it'll get the last comm anyway.
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/comm.c | 15 +++++++++++++++
tools/perf/util/comm.h | 1 +
tools/perf/util/hist.c | 3 +++
tools/perf/util/sort.c | 14 +++++---------
tools/perf/util/sort.h | 1 +
tools/perf/util/thread.c | 6 +++---
tools/perf/util/thread.h | 2 ++
7 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index 87f4a10e4a23..2d0f94f6593e 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -95,6 +95,21 @@ struct comm *comm__new(const char *str, u64 timestamp)
return self;
}
+void comm__override(struct comm *comm, const char *str, u64 timestamp)
+{
+ struct comm_str *old = comm->comm_str;
+
+ comm->comm_str = comm_str_findnew(str, &comm_str_root);
+ if (!comm->comm_str) {
+ comm->comm_str = old;
+ return;
+ }
+
+ comm->start = timestamp;
+ comm_str_get(comm->comm_str);
+ comm_str_put(old);
+}
+
void comm__free(struct comm *self)
{
comm_str_put(self->comm_str);
diff --git a/tools/perf/util/comm.h b/tools/perf/util/comm.h
index 2e47fb7e27de..d37c2898e665 100644
--- a/tools/perf/util/comm.h
+++ b/tools/perf/util/comm.h
@@ -16,5 +16,6 @@ struct comm {
void comm__free(struct comm *self);
struct comm *comm__new(const char *str, u64 timestamp);
const char *comm__str(struct comm *self);
+void comm__override(struct comm *self, const char *str, u64 timestamp);
#endif /* __PERF_COMM_H */
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1f5767f97dce..1fe90bd9dcb7 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -412,6 +412,7 @@ struct hist_entry *__hists__add_mem_entry(struct hists *self,
{
struct hist_entry entry = {
.thread = al->thread,
+ .comm = curr_comm(al->thread),
.ms = {
.map = al->map,
.sym = al->sym,
@@ -442,6 +443,7 @@ struct hist_entry *__hists__add_branch_entry(struct hists *self,
{
struct hist_entry entry = {
.thread = al->thread,
+ .comm = curr_comm(al->thread),
.ms = {
.map = bi->to.map,
.sym = bi->to.sym,
@@ -471,6 +473,7 @@ struct hist_entry *__hists__add_entry(struct hists *self,
{
struct hist_entry entry = {
.thread = al->thread,
+ .comm = curr_comm(al->thread),
.ms = {
.map = al->map,
.sym = al->sym,
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 3b307e740d6e..65f10784d2dc 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1,5 +1,6 @@
#include "sort.h"
#include "hist.h"
+#include "comm.h"
#include "symbol.h"
regex_t parent_regex;
@@ -80,25 +81,20 @@ static int64_t
sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
{
/* Compare the addr that should be unique among comm */
- return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
+ return comm__str(right->comm) - comm__str(left->comm);
}
static int64_t
sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
{
- const char *comm_l = thread__comm_curr(left->thread);
- const char *comm_r = thread__comm_curr(right->thread);
-
- if (!comm_l || !comm_r)
- return cmp_null(comm_l, comm_r);
-
- return strcmp(comm_l, comm_r);
+ /* Compare the addr that should be unique among comm */
+ return comm__str(right->comm) - comm__str(left->comm);
}
static int hist_entry__comm_snprintf(struct hist_entry *self, char *bf,
size_t size, unsigned int width)
{
- return repsep_snprintf(bf, size, "%*s", width, thread__comm_curr(self->thread));
+ return repsep_snprintf(bf, size, "%*s", width, comm__str(self->comm));
}
struct sort_entry sort_comm = {
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 4e80dbd271e7..4d0153f83e3c 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -84,6 +84,7 @@ struct hist_entry {
struct he_stat stat;
struct map_symbol ms;
struct thread *thread;
+ struct comm *comm;
u64 ip;
s32 cpu;
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index d3ca133329b2..00caed1abb5f 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -54,7 +54,7 @@ void thread__delete(struct thread *self)
free(self);
}
-static struct comm *curr_comm(const struct thread *self)
+struct comm *curr_comm(const struct thread *self)
{
if (list_empty(&self->comm_list))
return NULL;
@@ -69,8 +69,8 @@ int thread__set_comm(struct thread *self, const char *str, u64 timestamp)
/* Override latest entry if it had no specific time coverage */
if (!curr->start) {
- list_del(&curr->list);
- comm__free(curr);
+ comm__override(curr, str, timestamp);
+ return 0;
}
new = comm__new(str, timestamp);
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 1d9378abb515..cf8e31d0028b 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -26,6 +26,7 @@ struct thread {
};
struct machine;
+struct comm;
struct thread *thread__new(pid_t pid, pid_t tid);
void thread__delete(struct thread *self);
@@ -36,6 +37,7 @@ static inline void thread__exited(struct thread *thread)
int thread__set_comm(struct thread *self, const char *comm, u64 timestamp);
int thread__comm_len(struct thread *self);
+struct comm *curr_comm(const struct thread *self);
const char *thread__comm_curr(const struct thread *self);
void thread__insert_map(struct thread *self, struct map *map);
int thread__fork(struct thread *self, struct thread *parent, u64 timestamp);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-12 20:29 [PATCH 0/4] perf tools: New comm infrastructure Frederic Weisbecker
` (3 preceding siblings ...)
2013-09-12 20:29 ` [PATCH 4/4] perf tools: Compare hists comm by addresses Frederic Weisbecker
@ 2013-09-12 20:36 ` Ingo Molnar
2013-09-13 12:43 ` Frederic Weisbecker
2013-09-13 6:32 ` Namhyung Kim
5 siblings, 1 reply; 20+ messages in thread
From: Ingo Molnar @ 2013-09-12 20:36 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Namhyung Kim,
Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Linus Torvalds
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> The way we handle hists sorted by comm is to first gather them by tid
> then in the end merge/collapse hists that end up with the same comm.
>
> But merging hists has shown some performances issues, especially with
> callchain where the operation can be very heavy.
>
> So this new comm infrastructure aims at removing comm collapses. It
> brings two features:
>
> 1) Keep track of comms lifecycle by storing timestamps when the comms
> are set. This way we can map the precise comm to any thread:time couple.
> This only works if the PERF_SAMPLE_ID comes along comm and fork events,
> otherwise we only track the latest comm set for a thread.
>
> This can provide us more precise comm sorted hists by distinguishing pre
> and post exec timeframes into seperate hists for a single thread.
>
> Note that although the comm infrastructure is ready to do this, I
> haven't yet made the perf tools support that. It's a TODO entry.
>
> 2) Allocate comms only once instead of duplicating them for all threads
> sharing a same one. Two threads having the same comm should now point to
> the same string. As a result we can compare hists thread comm by
> address.
>
> The big upside is that we can now live sort comm hists instead of
> collapsing them in the end of the processing.
>
> I've seen very nice performance results on perf report. Roughly a 1.5x
> to 2x on perf report default stdio output with callchains.
>
> You can try this branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> perf/comm
>
> May be merging that with Namhyung callchains patches could provide some
> cumulative nice results.
It would be nice to try Linus's testcase, which is, in essence a kernel
build profile:
make defconfig
perf record -g make -j64 bzImage
and to make sure that it can analyze the data in same, non-annoying
runtimes. What I saw was 30 minutes of runtime - a 2x improvement is not
nearly enough, 15 minutes is still an eternity.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-12 20:36 ` [PATCH 0/4] perf tools: New comm infrastructure Ingo Molnar
@ 2013-09-13 12:43 ` Frederic Weisbecker
2013-09-13 15:59 ` David Ahern
2013-09-14 6:11 ` Ingo Molnar
0 siblings, 2 replies; 20+ messages in thread
From: Frederic Weisbecker @ 2013-09-13 12:43 UTC (permalink / raw)
To: Ingo Molnar
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Namhyung Kim,
Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Linus Torvalds
On Thu, Sep 12, 2013 at 10:36:58PM +0200, Ingo Molnar wrote:
>
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
> > The way we handle hists sorted by comm is to first gather them by tid
> > then in the end merge/collapse hists that end up with the same comm.
> >
> > But merging hists has shown some performances issues, especially with
> > callchain where the operation can be very heavy.
> >
> > So this new comm infrastructure aims at removing comm collapses. It
> > brings two features:
> >
> > 1) Keep track of comms lifecycle by storing timestamps when the comms
> > are set. This way we can map the precise comm to any thread:time couple.
> > This only works if the PERF_SAMPLE_ID comes along comm and fork events,
> > otherwise we only track the latest comm set for a thread.
> >
> > This can provide us more precise comm sorted hists by distinguishing pre
> > and post exec timeframes into seperate hists for a single thread.
> >
> > Note that although the comm infrastructure is ready to do this, I
> > haven't yet made the perf tools support that. It's a TODO entry.
> >
> > 2) Allocate comms only once instead of duplicating them for all threads
> > sharing a same one. Two threads having the same comm should now point to
> > the same string. As a result we can compare hists thread comm by
> > address.
> >
> > The big upside is that we can now live sort comm hists instead of
> > collapsing them in the end of the processing.
> >
> > I've seen very nice performance results on perf report. Roughly a 1.5x
> > to 2x on perf report default stdio output with callchains.
> >
> > You can try this branch:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > perf/comm
> >
> > May be merging that with Namhyung callchains patches could provide some
> > cumulative nice results.
>
> It would be nice to try Linus's testcase, which is, in essence a kernel
> build profile:
>
> make defconfig
> perf record -g make -j64 bzImage
>
> and to make sure that it can analyze the data in same, non-annoying
> runtimes. What I saw was 30 minutes of runtime - a 2x improvement is not
> nearly enough, 15 minutes is still an eternity.
I doubt we can reach anything near non-annonying runtimes after recording all the callchains
of a whole kernel build perf record.
My patches and Namhyung's should improve the comm situation a lot but we can't
do much miracle. The only way would be perhaps to be able to limit the deepness
of the callchain branches.
Now may be we can find other big contention point in perf. It's possible we also have
some endless loop somewhere.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-13 12:43 ` Frederic Weisbecker
@ 2013-09-13 15:59 ` David Ahern
2013-09-14 6:11 ` Ingo Molnar
1 sibling, 0 replies; 20+ messages in thread
From: David Ahern @ 2013-09-13 15:59 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: Ingo Molnar, LKML, Jiri Olsa, Ingo Molnar, Namhyung Kim,
Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Linus Torvalds
On 9/13/13 5:43 AM, Frederic Weisbecker wrote:
> The only way would be perhaps to be able to limit the deepness
> of the callchain branches.
I was thinking about such a feature two days ago. Max callchain depth is
set to 255 at compile time which can generate huge event sizes. perf
needs to allow the user to specify max depth for an event at less than that.
David
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-13 12:43 ` Frederic Weisbecker
2013-09-13 15:59 ` David Ahern
@ 2013-09-14 6:11 ` Ingo Molnar
2013-09-17 5:54 ` Namhyung Kim
1 sibling, 1 reply; 20+ messages in thread
From: Ingo Molnar @ 2013-09-14 6:11 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Namhyung Kim,
Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Linus Torvalds
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Thu, Sep 12, 2013 at 10:36:58PM +0200, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >
> > > The way we handle hists sorted by comm is to first gather them by tid
> > > then in the end merge/collapse hists that end up with the same comm.
> > >
> > > But merging hists has shown some performances issues, especially with
> > > callchain where the operation can be very heavy.
> > >
> > > So this new comm infrastructure aims at removing comm collapses. It
> > > brings two features:
> > >
> > > 1) Keep track of comms lifecycle by storing timestamps when the comms
> > > are set. This way we can map the precise comm to any thread:time couple.
> > > This only works if the PERF_SAMPLE_ID comes along comm and fork events,
> > > otherwise we only track the latest comm set for a thread.
> > >
> > > This can provide us more precise comm sorted hists by distinguishing pre
> > > and post exec timeframes into seperate hists for a single thread.
> > >
> > > Note that although the comm infrastructure is ready to do this, I
> > > haven't yet made the perf tools support that. It's a TODO entry.
> > >
> > > 2) Allocate comms only once instead of duplicating them for all threads
> > > sharing a same one. Two threads having the same comm should now point to
> > > the same string. As a result we can compare hists thread comm by
> > > address.
> > >
> > > The big upside is that we can now live sort comm hists instead of
> > > collapsing them in the end of the processing.
> > >
> > > I've seen very nice performance results on perf report. Roughly a 1.5x
> > > to 2x on perf report default stdio output with callchains.
> > >
> > > You can try this branch:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > > perf/comm
> > >
> > > May be merging that with Namhyung callchains patches could provide some
> > > cumulative nice results.
> >
> > It would be nice to try Linus's testcase, which is, in essence a kernel
> > build profile:
> >
> > make defconfig
> > perf record -g make -j64 bzImage
> >
> > and to make sure that it can analyze the data in same, non-annoying
> > runtimes. What I saw was 30 minutes of runtime - a 2x improvement is not
> > nearly enough, 15 minutes is still an eternity.
>
> I doubt we can reach anything near non-annonying runtimes after
> recording all the callchains of a whole kernel build perf record.
>
> My patches and Namhyung's should improve the comm situation a lot but we
> can't do much miracle. The only way would be perhaps to be able to limit
> the deepness of the callchain branches.
>
> Now may be we can find other big contention point in perf. It's possible
> we also have some endless loop somewhere.
Well, it was the 100,000+ step linear list walk that was causing 90% of
the slowness here. Namhyung's patch should dramatically improve that. I
guess time for someone to post a combined tree so that it can be tested
all together?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-14 6:11 ` Ingo Molnar
@ 2013-09-17 5:54 ` Namhyung Kim
2013-09-17 7:16 ` Ingo Molnar
2013-09-18 14:20 ` Arnaldo Carvalho de Melo
0 siblings, 2 replies; 20+ messages in thread
From: Namhyung Kim @ 2013-09-17 5:54 UTC (permalink / raw)
To: Ingo Molnar
Cc: Frederic Weisbecker, LKML, Jiri Olsa, David Ahern, Ingo Molnar,
Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Linus Torvalds
Hi Ingo,
On Sat, 14 Sep 2013 08:11:49 +0200, Ingo Molnar wrote:
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> My patches and Namhyung's should improve the comm situation a lot but we
>> can't do much miracle. The only way would be perhaps to be able to limit
>> the deepness of the callchain branches.
>>
>> Now may be we can find other big contention point in perf. It's possible
>> we also have some endless loop somewhere.
>
> Well, it was the 100,000+ step linear list walk that was causing 90% of
> the slowness here. Namhyung's patch should dramatically improve that. I
> guess time for someone to post a combined tree so that it can be tested
> all together?
I pushed combined tree to 'perf/callchain-v2' branch in my tree
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Please note that I also pushed other versions (v[1-3]). The v1 is my
previous rbtree conversion patch, v2 adds Frederic's new comm
infrastructure series on top and v3 adds my revised patch to refer
current comm [1] on top of v2.
I did my own test again among them. Test data is 400MB perf.data file
created by parallel kernel build.
$ ls -lh perf.data.big
-rw-------. 1 namhyung namhyung 400M Sep 9 10:21 perf.data.big
For more precise result, I changed cpufreq governor to 'performance'
# echo performance > /sys/devices/system/cpu/cpu3/cpufreq/scaling_governor
and run perf report on the cpu.
$ taskset -c 3 time -p perf --no-pager report --stdio -i perf.data.big > /dev/null
I ran it multiple times for each case and the results did not vary much.
baseline v1 v2 v3
----------------------------------------------------------
real 380.17 12.63 10.02 9.03
user 378.86 11.95 9.66 8.69
sys 0.70 0.65 0.33 0.34
I also tried to cache latest result and reuse it when adding a callchain
(in callchain_append() function) but it only hits ~5% and did not help
the performance.
Thanks,
Namhyung
[1] https://lkml.org/lkml/2013/9/16/565
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-17 5:54 ` Namhyung Kim
@ 2013-09-17 7:16 ` Ingo Molnar
2013-09-18 14:20 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 20+ messages in thread
From: Ingo Molnar @ 2013-09-17 7:16 UTC (permalink / raw)
To: Namhyung Kim
Cc: Frederic Weisbecker, LKML, Jiri Olsa, David Ahern, Ingo Molnar,
Peter Zijlstra, Arnaldo Carvalho de Melo, Stephane Eranian,
Linus Torvalds
* Namhyung Kim <namhyung@kernel.org> wrote:
> Hi Ingo,
>
> On Sat, 14 Sep 2013 08:11:49 +0200, Ingo Molnar wrote:
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> My patches and Namhyung's should improve the comm situation a lot but we
> >> can't do much miracle. The only way would be perhaps to be able to limit
> >> the deepness of the callchain branches.
> >>
> >> Now may be we can find other big contention point in perf. It's possible
> >> we also have some endless loop somewhere.
> >
> > Well, it was the 100,000+ step linear list walk that was causing 90% of
> > the slowness here. Namhyung's patch should dramatically improve that. I
> > guess time for someone to post a combined tree so that it can be tested
> > all together?
>
> I pushed combined tree to 'perf/callchain-v2' branch in my tree
>
> git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
>
> Please note that I also pushed other versions (v[1-3]). The v1 is my
> previous rbtree conversion patch, v2 adds Frederic's new comm
> infrastructure series on top and v3 adds my revised patch to refer
> current comm [1] on top of v2.
>
> I did my own test again among them. Test data is 400MB perf.data file
> created by parallel kernel build.
>
> $ ls -lh perf.data.big
> -rw-------. 1 namhyung namhyung 400M Sep 9 10:21 perf.data.big
>
> For more precise result, I changed cpufreq governor to 'performance'
Btw.,
>
> # echo performance > /sys/devices/system/cpu/cpu3/cpufreq/scaling_governor
>
> and run perf report on the cpu.
>
> $ taskset -c 3 time -p perf --no-pager report --stdio -i perf.data.big > /dev/null
Btw., for such things you could use 'perf stat --null --sync --repeat 3',
which will not use the PMU or even perf events, it only uses precise
timers to measure execution time:
$ taskset -c 3 perf stat --null --sync --repeat 3 -p perf --no-pager report --stdio -i perf.data.big > /dev/null
> I ran it multiple times for each case and the results did not vary much.
(perf stat --repeat will print a nice stddev as well.)
> baseline v1 v2 v3
> ----------------------------------------------------------
> real 380.17 12.63 10.02 9.03
> user 378.86 11.95 9.66 8.69
> sys 0.70 0.65 0.33 0.34
(Alas perf stat --null does not print a system/user time split. Might be
nice to implement that.)
The numbers look pretty nice, a 40x speedup. Especially with the progress
bar displayed this should be within a human-tolerable runtime.
Still it would be nice to look at some stats: number of records, number of
call chain entries, average call chain depth, tree size, max tree depth,
etc. - so that we get a processing cost estimation of how much we spend on
a single call chain entry, on average.
If any of those values is suspiciously high then maybe we could cull the
callchain depth by default, people rarely look beyond a couple of entries:
but this gets tricky when people sort in the reverse direction though - in
that case the deepest entries are just as valuable as well to the end
result.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-17 5:54 ` Namhyung Kim
2013-09-17 7:16 ` Ingo Molnar
@ 2013-09-18 14:20 ` Arnaldo Carvalho de Melo
2013-09-18 15:12 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2013-09-18 14:20 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Frederic Weisbecker, LKML, Jiri Olsa, David Ahern,
Ingo Molnar, Peter Zijlstra, Stephane Eranian, Linus Torvalds
Em Tue, Sep 17, 2013 at 02:54:22PM +0900, Namhyung Kim escreveu:
> Hi Ingo,
>
> On Sat, 14 Sep 2013 08:11:49 +0200, Ingo Molnar wrote:
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> My patches and Namhyung's should improve the comm situation a lot but we
> >> can't do much miracle. The only way would be perhaps to be able to limit
> >> the deepness of the callchain branches.
> >>
> >> Now may be we can find other big contention point in perf. It's possible
> >> we also have some endless loop somewhere.
> >
> > Well, it was the 100,000+ step linear list walk that was causing 90% of
> > the slowness here. Namhyung's patch should dramatically improve that. I
> > guess time for someone to post a combined tree so that it can be tested
> > all together?
>
> I pushed combined tree to 'perf/callchain-v2' branch in my tree
>
> git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
But you did it for some old tree, as I tried running it now and since my
big perf.data file has PERF_RECORD_MMAP2 records, I can't try it with
your branch, working on cherry picking those changesets, since a plain
rebase didn't work.
- Arnaldo
>
> Please note that I also pushed other versions (v[1-3]). The v1 is my
> previous rbtree conversion patch, v2 adds Frederic's new comm
> infrastructure series on top and v3 adds my revised patch to refer
> current comm [1] on top of v2.
>
> I did my own test again among them. Test data is 400MB perf.data file
> created by parallel kernel build.
>
> $ ls -lh perf.data.big
> -rw-------. 1 namhyung namhyung 400M Sep 9 10:21 perf.data.big
>
> For more precise result, I changed cpufreq governor to 'performance'
>
> # echo performance > /sys/devices/system/cpu/cpu3/cpufreq/scaling_governor
>
> and run perf report on the cpu.
>
> $ taskset -c 3 time -p perf --no-pager report --stdio -i perf.data.big > /dev/null
>
> I ran it multiple times for each case and the results did not vary much.
>
> baseline v1 v2 v3
> ----------------------------------------------------------
> real 380.17 12.63 10.02 9.03
> user 378.86 11.95 9.66 8.69
> sys 0.70 0.65 0.33 0.34
>
>
> I also tried to cache latest result and reuse it when adding a callchain
> (in callchain_append() function) but it only hits ~5% and did not help
> the performance.
>
> Thanks,
> Namhyung
>
>
> [1] https://lkml.org/lkml/2013/9/16/565
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-18 14:20 ` Arnaldo Carvalho de Melo
@ 2013-09-18 15:12 ` Arnaldo Carvalho de Melo
2013-09-18 15:58 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2013-09-18 15:12 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Frederic Weisbecker, LKML, Jiri Olsa, David Ahern,
Ingo Molnar, Peter Zijlstra, Stephane Eranian, Linus Torvalds
Em Wed, Sep 18, 2013 at 11:20:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 17, 2013 at 02:54:22PM +0900, Namhyung Kim escreveu:
> > On Sat, 14 Sep 2013 08:11:49 +0200, Ingo Molnar wrote:
> > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > >> My patches and Namhyung's should improve the comm situation a lot but we
> > >> can't do much miracle. The only way would be perhaps to be able to limit
> > >
> > >>
> > >> Now may be we can find other big contention point in perf. It's possible
> > >> we also have some endless loop somewhere.
> > >
> > > Well, it was the 100,000+ step linear list walk that was causing 90% of
> > > the slowness here. Namhyung's patch should dramatically improve that. I
> > > guess time for someone to post a combined tree so that it can be tested
> > > all together?
> > I pushed combined tree to 'perf/callchain-v2' branch in my tree
> > git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> But you did it for some old tree, as I tried running it now and since my
> big perf.data file has PERF_RECORD_MMAP2 records, I can't try it with
> your branch, working on cherry picking those changesets, since a plain
> rebase didn't work.
Cherry picking just:
commit ad3c9edd70122c8776bc0735287daba3945e328a
Author: Namhyung Kim <namhyung.kim@lge.com>
Date: Mon Sep 9 17:05:38 2013 +0900
perf callchain: Convert children list to rbtree
I go from too-long-to-wait to:
[root@sandy ~]# echo performance > /sys/devices/system/cpu/cpu3/cpufreq/scaling_governor
[acme@sandy linux]$ taskset -c 3 perf stat --null --sync --repeat 3 perf report -i perf.data.make-j8-kernel > /dev/null
Performance counter stats for 'perf report -i perf.data.make-j8-kernel' (3 runs):
79.158464654 seconds time elapsed ( +- 0.15% )
[acme@sandy linux]$
[acme@sandy linux]$ ls -lah perf.data.make-j8-kernel
-rw-------. 1 acme acme 2.3G Sep 16 11:56 perf.data.make-j8-kernel
Looking at the other patches, modulo the ones that do the progress bar updating
while collapsing, as they clash with a patch from Jiri Olsa.
- Arnaldo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-18 15:12 ` Arnaldo Carvalho de Melo
@ 2013-09-18 15:58 ` Arnaldo Carvalho de Melo
2013-09-18 16:17 ` Namhyung Kim
0 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2013-09-18 15:58 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Frederic Weisbecker, LKML, Jiri Olsa, David Ahern,
Ingo Molnar, Peter Zijlstra, Stephane Eranian, Linus Torvalds
Em Wed, Sep 18, 2013 at 12:12:53PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Sep 18, 2013 at 11:20:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Sep 17, 2013 at 02:54:22PM +0900, Namhyung Kim escreveu:
> > > On Sat, 14 Sep 2013 08:11:49 +0200, Ingo Molnar wrote:
> > > > Well, it was the 100,000+ step linear list walk that was causing 90% of
> > > > the slowness here. Namhyung's patch should dramatically improve that. I
> > > > guess time for someone to post a combined tree so that it can be tested
> > > > all together?
> Cherry picking just:
>
> perf callchain: Convert children list to rbtree
>
> I go from too-long-to-wait to:
>
> [root@sandy ~]# echo performance > /sys/devices/system/cpu/cpu3/cpufreq/scaling_governor
> [acme@sandy linux]$ taskset -c 3 perf stat --null --sync --repeat 3 perf report -i perf.data.make-j8-kernel > /dev/null
> Performance counter stats for 'perf report -i perf.data.make-j8-kernel' (3 runs):
> 79.158464654 seconds time elapsed ( +- 0.15% )
> [acme@sandy linux]$
> [acme@sandy linux]$ ls -lah perf.data.make-j8-kernel
> -rw-------. 1 acme acme 2.3G Sep 16 11:56 perf.data.make-j8-kernel
And with the others, fixed up to cope with PERF_RECORD_MMAP2, except the
progress bar updating:
[acme@sandy linux]$ taskset -c 3 perf stat --null --sync --repeat 3 perf report -i perf.data.make-j8-kernel > /dev/null
Performance counter stats for 'perf report -i perf.data.make-j8-kernel' (3 runs):
55.697701530 seconds time elapsed ( +- 0.13% )
[acme@sandy linux]$
Branch perf/callchain-v4 on my tree.
- Arnaldo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-18 15:58 ` Arnaldo Carvalho de Melo
@ 2013-09-18 16:17 ` Namhyung Kim
0 siblings, 0 replies; 20+ messages in thread
From: Namhyung Kim @ 2013-09-18 16:17 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Ingo Molnar, Frederic Weisbecker, LKML, Jiri Olsa, David Ahern,
Ingo Molnar, Peter Zijlstra, Stephane Eranian, Linus Torvalds
Hi Arnaldo,
Thanks for your work! I'm gonna be offline this week as it's one of
the biggest holyday in Korea (and other Asian countries). I'll catch
up next week.
--
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-12 20:29 [PATCH 0/4] perf tools: New comm infrastructure Frederic Weisbecker
` (4 preceding siblings ...)
2013-09-12 20:36 ` [PATCH 0/4] perf tools: New comm infrastructure Ingo Molnar
@ 2013-09-13 6:32 ` Namhyung Kim
2013-09-13 12:46 ` Frederic Weisbecker
5 siblings, 1 reply; 20+ messages in thread
From: Namhyung Kim @ 2013-09-13 6:32 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Peter Zijlstra,
Arnaldo Carvalho de Melo, Stephane Eranian
Hi Frederic,
On Thu, 12 Sep 2013 22:29:39 +0200, Frederic Weisbecker wrote:
> The way we handle hists sorted by comm is to first gather them by tid then
> in the end merge/collapse hists that end up with the same comm.
>
> But merging hists has shown some performances issues, especially with callchain
> where the operation can be very heavy.
>
> So this new comm infrastructure aims at removing comm collapses. It brings
> two features:
>
> 1) Keep track of comms lifecycle by storing timestamps when the comms
> are set. This way we can map the precise comm to any thread:time couple.
> This only works if the PERF_SAMPLE_ID comes along comm and fork events,
> otherwise we only track the latest comm set for a thread.
>
> This can provide us more precise comm sorted hists by distinguishing pre and
> post exec timeframes into seperate hists for a single thread.
>
> Note that although the comm infrastructure is ready to do this, I haven't
> yet made the perf tools support that. It's a TODO entry.
>
>
> 2) Allocate comms only once instead of duplicating them for all threads sharing
> a same one. Two threads having the same comm should now point to the same string.
> As a result we can compare hists thread comm by address.
>
> The big upside is that we can now live sort comm hists instead of collapsing
> them in the end of the processing.
>
> I've seen very nice performance results on perf report. Roughly a 1.5x to 2x
> on perf report default stdio output with callchains.
>
> You can try this branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> perf/comm
>
> May be merging that with Namhyung callchains patches could provide some
> cumulative nice results.
I got this:
ui/browsers/hists.c: In function ‘hists__browser_title’:
ui/browsers/hists.c:1258:10: error: passing argument 1 of ‘thread__comm_curr’ discards ‘const’ qualifier from pointer target type [-Werror]
In file included from ui/browsers/../../util/sort.h:24:0,
from ui/browsers/hists.c:11:
ui/browsers/../../util/thread.h:39:13: note: expected ‘struct thread *’ but argument is of type ‘const struct thread *’
ui/browsers/hists.c: In function ‘perf_evsel__hists_browse’:
ui/browsers/hists.c:1581:9: error: passing argument 1 of ‘thread__comm_curr’ discards ‘const’ qualifier from pointer target type [-Werror]
In file included from ui/browsers/../../util/sort.h:24:0,
from ui/browsers/hists.c:11:
ui/browsers/../../util/thread.h:39:13: note: expected ‘struct thread *’ but argument is of type ‘const struct thread *’
ui/browsers/hists.c:1704:10: error: passing argument 1 of ‘thread__comm_curr’ discards ‘const’ qualifier from pointer target type [-Werror]
In file included from ui/browsers/../../util/sort.h:24:0,
from ui/browsers/hists.c:11:
ui/browsers/../../util/thread.h:39:13: note: expected ‘struct thread *’ but argument is of type ‘const struct thread *’
cc1: all warnings being treated as errors
make: *** [ui/browsers/hists.o] Error 1
make: *** Waiting for unfinished jobs....
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 0/4] perf tools: New comm infrastructure
2013-09-13 6:32 ` Namhyung Kim
@ 2013-09-13 12:46 ` Frederic Weisbecker
0 siblings, 0 replies; 20+ messages in thread
From: Frederic Weisbecker @ 2013-09-13 12:46 UTC (permalink / raw)
To: Namhyung Kim
Cc: LKML, Jiri Olsa, David Ahern, Ingo Molnar, Peter Zijlstra,
Arnaldo Carvalho de Melo, Stephane Eranian
On Fri, Sep 13, 2013 at 03:32:34PM +0900, Namhyung Kim wrote:
> Hi Frederic,
>
> On Thu, 12 Sep 2013 22:29:39 +0200, Frederic Weisbecker wrote:
> > The way we handle hists sorted by comm is to first gather them by tid then
> > in the end merge/collapse hists that end up with the same comm.
> >
> > But merging hists has shown some performances issues, especially with callchain
> > where the operation can be very heavy.
> >
> > So this new comm infrastructure aims at removing comm collapses. It brings
> > two features:
> >
> > 1) Keep track of comms lifecycle by storing timestamps when the comms
> > are set. This way we can map the precise comm to any thread:time couple.
> > This only works if the PERF_SAMPLE_ID comes along comm and fork events,
> > otherwise we only track the latest comm set for a thread.
> >
> > This can provide us more precise comm sorted hists by distinguishing pre and
> > post exec timeframes into seperate hists for a single thread.
> >
> > Note that although the comm infrastructure is ready to do this, I haven't
> > yet made the perf tools support that. It's a TODO entry.
> >
> >
> > 2) Allocate comms only once instead of duplicating them for all threads sharing
> > a same one. Two threads having the same comm should now point to the same string.
> > As a result we can compare hists thread comm by address.
> >
> > The big upside is that we can now live sort comm hists instead of collapsing
> > them in the end of the processing.
> >
> > I've seen very nice performance results on perf report. Roughly a 1.5x to 2x
> > on perf report default stdio output with callchains.
> >
> > You can try this branch:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > perf/comm
> >
> > May be merging that with Namhyung callchains patches could provide some
> > cumulative nice results.
>
> I got this:
>
> ui/browsers/hists.c: In function ‘hists__browser_title’:
> ui/browsers/hists.c:1258:10: error: passing argument 1 of ‘thread__comm_curr’ discards ‘const’ qualifier from pointer target type [-Werror]
> In file included from ui/browsers/../../util/sort.h:24:0,
> from ui/browsers/hists.c:11:
> ui/browsers/../../util/thread.h:39:13: note: expected ‘struct thread *’ but argument is of type ‘const struct thread *’
> ui/browsers/hists.c: In function ‘perf_evsel__hists_browse’:
> ui/browsers/hists.c:1581:9: error: passing argument 1 of ‘thread__comm_curr’ discards ‘const’ qualifier from pointer target type [-Werror]
> In file included from ui/browsers/../../util/sort.h:24:0,
> from ui/browsers/hists.c:11:
> ui/browsers/../../util/thread.h:39:13: note: expected ‘struct thread *’ but argument is of type ‘const struct thread *’
> ui/browsers/hists.c:1704:10: error: passing argument 1 of ‘thread__comm_curr’ discards ‘const’ qualifier from pointer target type [-Werror]
> In file included from ui/browsers/../../util/sort.h:24:0,
> from ui/browsers/hists.c:11:
> ui/browsers/../../util/thread.h:39:13: note: expected ‘struct thread *’ but argument is of type ‘const struct thread *’
> cc1: all warnings being treated as errors
> make: *** [ui/browsers/hists.o] Error 1
> make: *** Waiting for unfinished jobs....
Oops, I'm missing the libs to build the ui, so I didn't see this. Will fix, thanks!
>
> Thanks,
> Namhyung
^ permalink raw reply [flat|nested] 20+ messages in thread