* [PATCH 1/3] perf top: Get rid of *_threaded() functions
@ 2013-05-13 6:16 Namhyung Kim
2013-05-13 6:16 ` [PATCH 2/3] perf hists: Move locking to its call-sites Namhyung Kim
2013-05-13 6:16 ` [PATCH 3/3] perf report: Don't be bothered with locking when adding hist entries Namhyung Kim
0 siblings, 2 replies; 3+ messages in thread
From: Namhyung Kim @ 2013-05-13 6:16 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
Jiri Olsa, David Ahern, Stephane Eranian, Andi Kleen
From: Namhyung Kim <namhyung.kim@lge.com>
Those _threaded() functions are needed to make hist tree handling
thread-safe, but AFAICS the only thing it does is forcing it to use
the intermediate 'collapsed' tree. It can be acheived by setting
sort__need_collapse to 1 in cmd_top() so no need to keep those
_threaded() variants.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-top.c | 23 +++++++++++++----------
tools/perf/util/hist.c | 44 ++++++--------------------------------------
tools/perf/util/hist.h | 4 ----
3 files changed, 19 insertions(+), 52 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 81adcafbac8f..5cd41ec43ce1 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -284,11 +284,11 @@ static void perf_top__print_sym_table(struct perf_top *top)
return;
}
- hists__collapse_resort_threaded(&top->sym_evsel->hists);
- hists__output_resort_threaded(&top->sym_evsel->hists);
- hists__decay_entries_threaded(&top->sym_evsel->hists,
- top->hide_user_symbols,
- top->hide_kernel_symbols);
+ hists__collapse_resort(&top->sym_evsel->hists);
+ hists__output_resort(&top->sym_evsel->hists);
+ hists__decay_entries(&top->sym_evsel->hists,
+ top->hide_user_symbols,
+ top->hide_kernel_symbols);
hists__output_recalc_col_len(&top->sym_evsel->hists,
top->print_entries - printed);
putchar('\n');
@@ -549,11 +549,11 @@ static void perf_top__sort_new_samples(void *arg)
if (t->evlist->selected != NULL)
t->sym_evsel = t->evlist->selected;
- hists__collapse_resort_threaded(&t->sym_evsel->hists);
- hists__output_resort_threaded(&t->sym_evsel->hists);
- hists__decay_entries_threaded(&t->sym_evsel->hists,
- t->hide_user_symbols,
- t->hide_kernel_symbols);
+ hists__collapse_resort(&t->sym_evsel->hists);
+ hists__output_resort(&t->sym_evsel->hists);
+ hists__decay_entries(&t->sym_evsel->hists,
+ t->hide_user_symbols,
+ t->hide_kernel_symbols);
}
static void *display_thread_tui(void *arg)
@@ -1126,6 +1126,9 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
if (setup_sorting() < 0)
usage_with_options(top_usage, options);
+ /* display thread wants entries to be collapsed in a different tree */
+ sort__need_collapse = 1;
+
if (top.use_stdio)
use_browser = 0;
else if (top.use_tui)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 72b4eec820c3..7e0fa628e9ab 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -240,8 +240,7 @@ static bool hists__decay_entry(struct hists *hists, struct hist_entry *he)
return he->stat.period == 0;
}
-static void __hists__decay_entries(struct hists *hists, bool zap_user,
- bool zap_kernel, bool threaded)
+void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel)
{
struct rb_node *next = rb_first(&hists->entries);
struct hist_entry *n;
@@ -260,7 +259,7 @@ static void __hists__decay_entries(struct hists *hists, bool zap_user,
!n->used) {
rb_erase(&n->rb_node, &hists->entries);
- if (sort__need_collapse || threaded)
+ if (sort__need_collapse)
rb_erase(&n->rb_node_in, &hists->entries_collapsed);
hist_entry__free(n);
@@ -269,17 +268,6 @@ static void __hists__decay_entries(struct hists *hists, bool zap_user,
}
}
-void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel)
-{
- return __hists__decay_entries(hists, zap_user, zap_kernel, false);
-}
-
-void hists__decay_entries_threaded(struct hists *hists,
- bool zap_user, bool zap_kernel)
-{
- return __hists__decay_entries(hists, zap_user, zap_kernel, true);
-}
-
/*
* histogram, sorted on item, collects periods
*/
@@ -613,13 +601,13 @@ static void hists__apply_filters(struct hists *hists, struct hist_entry *he)
hists__filter_entry_by_symbol(hists, he);
}
-static void __hists__collapse_resort(struct hists *hists, bool threaded)
+void hists__collapse_resort(struct hists *hists)
{
struct rb_root *root;
struct rb_node *next;
struct hist_entry *n;
- if (!sort__need_collapse && !threaded)
+ if (!sort__need_collapse)
return;
root = hists__get_rotate_entries_in(hists);
@@ -641,16 +629,6 @@ static void __hists__collapse_resort(struct hists *hists, bool threaded)
}
}
-void hists__collapse_resort(struct hists *hists)
-{
- return __hists__collapse_resort(hists, false);
-}
-
-void hists__collapse_resort_threaded(struct hists *hists)
-{
- return __hists__collapse_resort(hists, true);
-}
-
/*
* reverse the map, sort on period.
*/
@@ -737,7 +715,7 @@ static void __hists__insert_output_entry(struct rb_root *entries,
rb_insert_color(&he->rb_node, entries);
}
-static void __hists__output_resort(struct hists *hists, bool threaded)
+void hists__output_resort(struct hists *hists)
{
struct rb_root *root;
struct rb_node *next;
@@ -746,7 +724,7 @@ static void __hists__output_resort(struct hists *hists, bool threaded)
min_callchain_hits = hists->stats.total_period * (callchain_param.min_percent / 100);
- if (sort__need_collapse || threaded)
+ if (sort__need_collapse)
root = &hists->entries_collapsed;
else
root = hists->entries_in;
@@ -767,16 +745,6 @@ static void __hists__output_resort(struct hists *hists, bool threaded)
}
}
-void hists__output_resort(struct hists *hists)
-{
- return __hists__output_resort(hists, false);
-}
-
-void hists__output_resort_threaded(struct hists *hists)
-{
- return __hists__output_resort(hists, true);
-}
-
static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h,
enum hist_filter filter)
{
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 6be88dc12b9a..bd81d799a1bf 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -104,13 +104,9 @@ struct hist_entry *__hists__add_mem_entry(struct hists *self,
u64 weight);
void hists__output_resort(struct hists *self);
-void hists__output_resort_threaded(struct hists *hists);
void hists__collapse_resort(struct hists *self);
-void hists__collapse_resort_threaded(struct hists *hists);
void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);
-void hists__decay_entries_threaded(struct hists *hists, bool zap_user,
- bool zap_kernel);
void hists__output_recalc_col_len(struct hists *hists, int max_rows);
void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h);
--
1.7.11.7
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 2/3] perf hists: Move locking to its call-sites
2013-05-13 6:16 [PATCH 1/3] perf top: Get rid of *_threaded() functions Namhyung Kim
@ 2013-05-13 6:16 ` Namhyung Kim
2013-05-13 6:16 ` [PATCH 3/3] perf report: Don't be bothered with locking when adding hist entries Namhyung Kim
1 sibling, 0 replies; 3+ messages in thread
From: Namhyung Kim @ 2013-05-13 6:16 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
Jiri Olsa, David Ahern, Stephane Eranian, Andi Kleen
From: Namhyung Kim <namhyung.kim@lge.com>
It's a preparation patch to eliminate unneeded locking in the perf
report path.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-report.c | 26 ++++++++++++++------------
tools/perf/builtin-top.c | 3 +++
tools/perf/util/hist.c | 6 +-----
3 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index d45bf9b0361d..63febd24e912 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -297,6 +297,7 @@ static int process_sample_event(struct perf_tool *tool,
{
struct perf_report *rep = container_of(tool, struct perf_report, tool);
struct addr_location al;
+ int ret;
if (perf_event__preprocess_sample(event, machine, &al, sample,
rep->annotate_init) < 0) {
@@ -311,28 +312,29 @@ static int process_sample_event(struct perf_tool *tool,
if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
return 0;
+ pthread_mutex_lock(&evsel->hists.lock);
+
if (sort__mode == SORT_MODE__BRANCH) {
- if (perf_report__add_branch_hist_entry(tool, &al, sample,
- evsel, machine)) {
+ ret = perf_report__add_branch_hist_entry(tool, &al, sample,
+ evsel, machine);
+ if (ret < 0)
pr_debug("problem adding lbr entry, skipping event\n");
- return -1;
- }
} else if (rep->mem_mode == 1) {
- if (perf_report__add_mem_hist_entry(tool, &al, sample,
- evsel, machine, event)) {
+ ret = perf_report__add_mem_hist_entry(tool, &al, sample,
+ evsel, machine, event);
+ if (ret < 0)
pr_debug("problem adding mem entry, skipping event\n");
- return -1;
- }
} else {
if (al.map != NULL)
al.map->dso->hit = 1;
- if (perf_evsel__add_hist_entry(evsel, &al, sample, machine)) {
+ ret = perf_evsel__add_hist_entry(evsel, &al, sample, machine);
+ if (ret < 0)
pr_debug("problem incrementing symbol period, skipping event\n");
- return -1;
- }
}
- return 0;
+ pthread_mutex_unlock(&evsel->hists.lock);
+
+ return ret;
}
static int process_read_event(struct perf_tool *tool,
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 5cd41ec43ce1..c2c973476479 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -245,8 +245,11 @@ static struct hist_entry *perf_evsel__add_hist_entry(struct perf_evsel *evsel,
{
struct hist_entry *he;
+ pthread_mutex_lock(&evsel->hists.lock);
he = __hists__add_entry(&evsel->hists, al, NULL, sample->period,
sample->weight);
+ pthread_mutex_unlock(&evsel->hists.lock);
+
if (he == NULL)
return NULL;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 7e0fa628e9ab..b11a6cfdb414 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -347,8 +347,6 @@ static struct hist_entry *add_hist_entry(struct hists *hists,
struct hist_entry *he;
int cmp;
- pthread_mutex_lock(&hists->lock);
-
p = &hists->entries_in->rb_node;
while (*p != NULL) {
@@ -394,14 +392,12 @@ static struct hist_entry *add_hist_entry(struct hists *hists,
he = hist_entry__new(entry);
if (!he)
- goto out_unlock;
+ return NULL;
rb_link_node(&he->rb_node_in, parent, p);
rb_insert_color(&he->rb_node_in, hists->entries_in);
out:
hist_entry__add_cpumode_period(he, al->cpumode, period);
-out_unlock:
- pthread_mutex_unlock(&hists->lock);
return he;
}
--
1.7.11.7
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 3/3] perf report: Don't be bothered with locking when adding hist entries
2013-05-13 6:16 [PATCH 1/3] perf top: Get rid of *_threaded() functions Namhyung Kim
2013-05-13 6:16 ` [PATCH 2/3] perf hists: Move locking to its call-sites Namhyung Kim
@ 2013-05-13 6:16 ` Namhyung Kim
1 sibling, 0 replies; 3+ messages in thread
From: Namhyung Kim @ 2013-05-13 6:16 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
Jiri Olsa, David Ahern, Stephane Eranian, Andi Kleen
From: Namhyung Kim <namhyung.kim@lge.com>
The perf report is single-threaded, so no need to grab a lock.
Although the fast path of pthread_mutex_[un]lock() is very fast,
there's ~3% gain by eliminating it when we have huge sample data.
$ perf record -a -F 100000 -o perf.data.bench -- perf bench sched all
$ perf record -e cycles:upp -o perf.data.before -- \
> perf report -i perf.data.bench --stdio > /dev/null
... apply this patch ...
$ perf record -e cycles:upp -o perf.data.after -- \
> perf report -i perf.data.bench --stdio > /dev/null
$ perf diff perf.data.{before,after} | grep pthread
+0.02% libpthread-2.15.so [.] _pthread_cleanup_push_defer
+0.02% libpthread-2.15.so [.] _pthread_cleanup_pop_restore
0.05% -0.05% perf [.] pthread_mutex_unlock@plt
0.05% -0.05% perf [.] pthread_mutex_lock@plt
1.01% -1.01% libpthread-2.15.so [.] pthread_mutex_lock
1.68% -1.68% libpthread-2.15.so [.] __pthread_mutex_unlock_usercnt
0.05% -0.05% libpthread-2.15.so [.] pthread_mutex_unlock
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-report.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 63febd24e912..0f0cf2472d9d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -312,8 +312,6 @@ static int process_sample_event(struct perf_tool *tool,
if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
return 0;
- pthread_mutex_lock(&evsel->hists.lock);
-
if (sort__mode == SORT_MODE__BRANCH) {
ret = perf_report__add_branch_hist_entry(tool, &al, sample,
evsel, machine);
@@ -332,8 +330,6 @@ static int process_sample_event(struct perf_tool *tool,
if (ret < 0)
pr_debug("problem incrementing symbol period, skipping event\n");
}
- pthread_mutex_unlock(&evsel->hists.lock);
-
return ret;
}
--
1.7.11.7
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-05-13 6:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-13 6:16 [PATCH 1/3] perf top: Get rid of *_threaded() functions Namhyung Kim
2013-05-13 6:16 ` [PATCH 2/3] perf hists: Move locking to its call-sites Namhyung Kim
2013-05-13 6:16 ` [PATCH 3/3] perf report: Don't be bothered with locking when adding hist entries Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®