Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1354136 > unrolled thread
| Started by | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| First post | 2016-03-09 14:50 +0100 |
| Last post | 2016-03-10 11:40 +0100 |
| Articles | 12 — 3 participants |
Back to article view | Back to linux.kernel
[PATCHSET 0/7] perf tools: Fix and cleanups for hierarchy mode (v2) Namhyung Kim <namhyung@kernel.org> - 2016-03-09 14:50 +0100
[PATCH v2 5/7] perf hists browser: Cleanup hist_browser__fprintf_hierarchy_entry() Namhyung Kim <namhyung@kernel.org> - 2016-03-09 14:50 +0100
[tip:perf/core] perf hists browser: Cleanup hist_browser__fprintf_hierarchy_entry() tip-bot for Namhyung Kim <tipbot@zytor.com> - 2016-03-11 10:00 +0100
[PATCH v2 7/7] perf tools: Recalc total periods using top-level entries in hierarchy Namhyung Kim <namhyung@kernel.org> - 2016-03-09 14:50 +0100
[tip:perf/core] perf tools: Recalc total periods using top-level entries in hierarchy tip-bot for Namhyung Kim <tipbot@zytor.com> - 2016-03-11 10:00 +0100
[PATCH v2 1/7] perf tools: Fix hist_entry__filter() for hierarchy Namhyung Kim <namhyung@kernel.org> - 2016-03-09 14:50 +0100
[tip:perf/core] perf tools: Fix hist_entry__filter() for hierarchy tip-bot for Namhyung Kim <tipbot@zytor.com> - 2016-03-11 09:50 +0100
[PATCH v2 3/7] perf tools: Fix command line filters in hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-03-09 15:00 +0100
[tip:perf/core] perf tools: Fix command line filters in hierarchy mode tip-bot for Namhyung Kim <tipbot@zytor.com> - 2016-03-11 09:50 +0100
[PATCH v2 4/7] perf tools: Remove hist_entry->fmt field Namhyung Kim <namhyung@kernel.org> - 2016-03-09 15:00 +0100
[tip:perf/core] perf tools: Remove hist_entry->fmt field tip-bot for Namhyung Kim <tipbot@zytor.com> - 2016-03-11 10:00 +0100
Re: [PATCHSET 0/7] perf tools: Fix and cleanups for hierarchy mode (v2) Jiri Olsa <jolsa@redhat.com> - 2016-03-10 11:40 +0100
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-03-09 14:50 +0100 |
| Subject | [PATCHSET 0/7] perf tools: Fix and cleanups for hierarchy mode (v2) |
| Message-ID | <raMEW-9f-7@gated-at.bofh.it> |
Hello,
These are filter-related fixes and cleanups in the new hierarchy mode.
Currently perf misbehaves in hierarchy mode when filter is applied.
For example, it sometimes misses some (upper level) entries in the
output or shows incorrect (very large) overhead.
* Changes in v2)
- update hist_entry__filter() to check multiple filters properly
- add missing runtime filter check when adding hierarchy entries
- recalculate total period using top-level entries (Jiri)
- add more comment
Before:
$ perf report --hierarchy -s 'cpu,{dso,comm}' --comms swapper --stdio
...
# Overhead CPU / Shared Object+Command
# ........... ...........................
#
13.79% [kernel.vmlinux] swapper
31.71% 000
13.80% [kernel.vmlinux] swapper
0.43% [e1000e] swapper
11.89% [kernel.vmlinux] swapper
9.18% [kernel.vmlinux] swapper
After:
# Overhead CPU / Shared Object+Command
# ........... ...............................
#
33.09% 003
13.79% [kernel.vmlinux] swapper
31.71% 000
13.80% [kernel.vmlinux] swapper
0.43% [e1000e] swapper
21.90% 002
11.89% [kernel.vmlinux] swapper
13.30% 001
9.18% [kernel.vmlinux] swapper
It's also available on 'perf/hierarchy-filter-v2' branch in my tree
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
any feedback are welcome.
Thanks,
Namhyung
Namhyung Kim (7):
perf tools: Fix hist_entry__filter() for hierarchy
perf tools: Add more sort entry check functions
perf tools: Fix command line filters in hierarchy mode
perf tools: Remove hist_entry->fmt field
perf hists browser: Cleanup hist_browser__fprintf_hierarchy_entry()
perf tools: Remove nr_sort_keys field
perf tools: Recalc total periods using top-level entries in hierarchy
tools/perf/ui/browsers/hists.c | 22 +++----
tools/perf/ui/hist.c | 3 -
tools/perf/util/hist.c | 144 +++++++++++++++++++++++++++++++++++++----
tools/perf/util/hist.h | 6 +-
tools/perf/util/sort.c | 104 ++++++++++++-----------------
tools/perf/util/sort.h | 1 -
6 files changed, 183 insertions(+), 97 deletions(-)
--
2.7.2
[toc] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-03-09 14:50 +0100 |
| Subject | [PATCH v2 5/7] perf hists browser: Cleanup hist_browser__fprintf_hierarchy_entry() |
| Message-ID | <raMEW-9f-21@gated-at.bofh.it> |
| In reply to | #1354136 |
The hist_browser__fprintf_hierarchy_entry() if to dump current output
into a file so it needs to be sync-ed with the corresponding function
hist_browser__show_hierarchy_entry(). So use hists->nr_hpp_node to
indent width and use first fmt_node to print overhead columns instead of
checking whether it's a sort entry (or dynamic entry).
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/hists.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index e0e217ec856b..aed9c8f011f7 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1928,8 +1928,7 @@ static int hist_browser__fprintf_entry(struct hist_browser *browser,
static int hist_browser__fprintf_hierarchy_entry(struct hist_browser *browser,
struct hist_entry *he,
- FILE *fp, int level,
- int nr_sort_keys)
+ FILE *fp, int level)
{
char s[8192];
int printed = 0;
@@ -1939,23 +1938,20 @@ static int hist_browser__fprintf_hierarchy_entry(struct hist_browser *browser,
.size = sizeof(s),
};
struct perf_hpp_fmt *fmt;
+ struct perf_hpp_list_node *fmt_node;
bool first = true;
int ret;
- int hierarchy_indent = nr_sort_keys * HIERARCHY_INDENT;
+ int hierarchy_indent = (he->hists->nr_hpp_node - 2) * HIERARCHY_INDENT;
printed = fprintf(fp, "%*s", level * HIERARCHY_INDENT, "");
folded_sign = hist_entry__folded(he);
printed += fprintf(fp, "%c", folded_sign);
- hists__for_each_format(he->hists, fmt) {
- if (perf_hpp__should_skip(fmt, he->hists))
- continue;
-
- if (perf_hpp__is_sort_entry(fmt) ||
- perf_hpp__is_dynamic_entry(fmt))
- break;
-
+ /* the first hpp_list_node is for overhead columns */
+ fmt_node = list_first_entry(&he->hists->hpp_formats,
+ struct perf_hpp_list_node, list);
+ perf_hpp_list__for_each_format(&fmt_node->hpp, fmt) {
if (!first) {
ret = scnprintf(hpp.buf, hpp.size, " ");
advance_hpp(&hpp, ret);
@@ -1992,7 +1988,6 @@ static int hist_browser__fprintf(struct hist_browser *browser, FILE *fp)
struct rb_node *nd = hists__filter_entries(rb_first(browser->b.entries),
browser->min_pcnt);
int printed = 0;
- int nr_sort = browser->hists->nr_sort_keys;
while (nd) {
struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
@@ -2000,8 +1995,7 @@ static int hist_browser__fprintf(struct hist_browser *browser, FILE *fp)
if (symbol_conf.report_hierarchy) {
printed += hist_browser__fprintf_hierarchy_entry(browser,
h, fp,
- h->depth,
- nr_sort);
+ h->depth);
} else {
printed += hist_browser__fprintf_entry(browser, h, fp);
}
--
2.7.2
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Namhyung Kim <tipbot@zytor.com> |
|---|---|
| Date | 2016-03-11 10:00 +0100 |
| Subject | [tip:perf/core] perf hists browser: Cleanup hist_browser__fprintf_hierarchy_entry() |
| Message-ID | <rbr5p-3zy-25@gated-at.bofh.it> |
| In reply to | #1354137 |
Commit-ID: 325a62834e81452d2a6e253444022cf493bbabfc
Gitweb: http://git.kernel.org/tip/325a62834e81452d2a6e253444022cf493bbabfc
Author: Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 9 Mar 2016 22:47:00 +0900
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Mar 2016 16:46:04 -0300
perf hists browser: Cleanup hist_browser__fprintf_hierarchy_entry()
The hist_browser__fprintf_hierarchy_entry() if to dump current output
into a file so it needs to be sync-ed with the corresponding function
hist_browser__show_hierarchy_entry(). So use hists->nr_hpp_node to
indent width and use first fmt_node to print overhead columns instead of
checking whether it's a sort entry (or dynamic entry).
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1457531222-18130-6-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/ui/browsers/hists.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index e0e217e..aed9c8f 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1928,8 +1928,7 @@ static int hist_browser__fprintf_entry(struct hist_browser *browser,
static int hist_browser__fprintf_hierarchy_entry(struct hist_browser *browser,
struct hist_entry *he,
- FILE *fp, int level,
- int nr_sort_keys)
+ FILE *fp, int level)
{
char s[8192];
int printed = 0;
@@ -1939,23 +1938,20 @@ static int hist_browser__fprintf_hierarchy_entry(struct hist_browser *browser,
.size = sizeof(s),
};
struct perf_hpp_fmt *fmt;
+ struct perf_hpp_list_node *fmt_node;
bool first = true;
int ret;
- int hierarchy_indent = nr_sort_keys * HIERARCHY_INDENT;
+ int hierarchy_indent = (he->hists->nr_hpp_node - 2) * HIERARCHY_INDENT;
printed = fprintf(fp, "%*s", level * HIERARCHY_INDENT, "");
folded_sign = hist_entry__folded(he);
printed += fprintf(fp, "%c", folded_sign);
- hists__for_each_format(he->hists, fmt) {
- if (perf_hpp__should_skip(fmt, he->hists))
- continue;
-
- if (perf_hpp__is_sort_entry(fmt) ||
- perf_hpp__is_dynamic_entry(fmt))
- break;
-
+ /* the first hpp_list_node is for overhead columns */
+ fmt_node = list_first_entry(&he->hists->hpp_formats,
+ struct perf_hpp_list_node, list);
+ perf_hpp_list__for_each_format(&fmt_node->hpp, fmt) {
if (!first) {
ret = scnprintf(hpp.buf, hpp.size, " ");
advance_hpp(&hpp, ret);
@@ -1992,7 +1988,6 @@ static int hist_browser__fprintf(struct hist_browser *browser, FILE *fp)
struct rb_node *nd = hists__filter_entries(rb_first(browser->b.entries),
browser->min_pcnt);
int printed = 0;
- int nr_sort = browser->hists->nr_sort_keys;
while (nd) {
struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
@@ -2000,8 +1995,7 @@ static int hist_browser__fprintf(struct hist_browser *browser, FILE *fp)
if (symbol_conf.report_hierarchy) {
printed += hist_browser__fprintf_hierarchy_entry(browser,
h, fp,
- h->depth,
- nr_sort);
+ h->depth);
} else {
printed += hist_browser__fprintf_entry(browser, h, fp);
}
[toc] | [prev] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-03-09 14:50 +0100 |
| Subject | [PATCH v2 7/7] perf tools: Recalc total periods using top-level entries in hierarchy |
| Message-ID | <raMEX-9f-25@gated-at.bofh.it> |
| In reply to | #1354136 |
When hierarchy mode is enabled, each entries in a same hierarchy shares
period. IOW an upper level entry's period is a sum of lower level
entries. Thus perf uses only one of them to calculate the total period
of hists. It was lowest-level (leaf) entries but it has a problem when
it comes to filters.
If a filter is applied, entries in the same level will be filtered or
not. But upper level entries still have period of their sum including
filtered one. So total sum of upper level entries will not be same as
sum of lower level entries.
This resulted in entries having more than 100% of overhead and it can be
produced using perf top with filter(s).
Reported-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/hist.c | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index a98f9345f686..290b3cbf6877 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1453,6 +1453,31 @@ void hists__inc_stats(struct hists *hists, struct hist_entry *h)
hists->stats.total_period += h->stat.period;
}
+static void hierarchy_recalc_total_periods(struct hists *hists)
+{
+ struct rb_node *node;
+ struct hist_entry *he;
+
+ node = rb_first(&hists->entries);
+
+ hists->stats.total_period = 0;
+ hists->stats.total_non_filtered_period = 0;
+
+ /*
+ * recalculate total period using top-level entries only
+ * since lower level entries only see non-filtered entries
+ * but upper level entries have sum of both entries.
+ */
+ while (node) {
+ he = rb_entry(node, struct hist_entry, rb_node);
+ node = rb_next(node);
+
+ hists->stats.total_period += he->stat.period;
+ if (!he->filtered)
+ hists->stats.total_non_filtered_period += he->stat.period;
+ }
+}
+
static void hierarchy_insert_output_entry(struct rb_root *root,
struct hist_entry *he)
{
@@ -1518,11 +1543,6 @@ static void hists__hierarchy_output_resort(struct hists *hists,
continue;
}
- /* only update stat for leaf entries to avoid duplication */
- hists__inc_stats(hists, he);
- if (!he->filtered)
- hists__calc_col_len(hists, he);
-
if (!use_callchain)
continue;
@@ -1602,11 +1622,13 @@ static void output_resort(struct hists *hists, struct ui_progress *prog,
hists__reset_col_len(hists);
if (symbol_conf.report_hierarchy) {
- return hists__hierarchy_output_resort(hists, prog,
- &hists->entries_collapsed,
- &hists->entries,
- min_callchain_hits,
- use_callchain);
+ hists__hierarchy_output_resort(hists, prog,
+ &hists->entries_collapsed,
+ &hists->entries,
+ min_callchain_hits,
+ use_callchain);
+ hierarchy_recalc_total_periods(hists);
+ return;
}
if (sort__need_collapse)
@@ -1927,6 +1949,8 @@ static void hists__filter_hierarchy(struct hists *hists, int type, const void *a
}
}
+ hierarchy_recalc_total_periods(hists);
+
/*
* resort output after applying a new filter since filter in a lower
* hierarchy can change periods in a upper hierarchy.
--
2.7.2
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Namhyung Kim <tipbot@zytor.com> |
|---|---|
| Date | 2016-03-11 10:00 +0100 |
| Subject | [tip:perf/core] perf tools: Recalc total periods using top-level entries in hierarchy |
| Message-ID | <rbr5r-3zy-71@gated-at.bofh.it> |
| In reply to | #1354138 |
Commit-ID: f7fb538afea55383a9383dac5c56887c601af5f4
Gitweb: http://git.kernel.org/tip/f7fb538afea55383a9383dac5c56887c601af5f4
Author: Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 9 Mar 2016 22:47:02 +0900
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Mar 2016 16:46:13 -0300
perf tools: Recalc total periods using top-level entries in hierarchy
When hierarchy mode is enabled, each entry in a hierarchy level shares
the period. IOW an upper level entry's period is the sum of lower level
entries. Thus perf uses only one of them to calculate the total period
of hists. It was lowest-level (leaf) entries but it has a problem when
it comes to filters.
If a filter is applied, entries in the same level will be filtered or
not. But upper level entries still have period of their sum including
filtered one. So total sum of upper level entries will not be same as
sum of lower level entries.
This resulted in entries having more than 100% of overhead and it can be
produced using perf top with filter(s).
Reported-and-Tested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1457531222-18130-8-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/hist.c | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index a98f934..290b3cb 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1453,6 +1453,31 @@ void hists__inc_stats(struct hists *hists, struct hist_entry *h)
hists->stats.total_period += h->stat.period;
}
+static void hierarchy_recalc_total_periods(struct hists *hists)
+{
+ struct rb_node *node;
+ struct hist_entry *he;
+
+ node = rb_first(&hists->entries);
+
+ hists->stats.total_period = 0;
+ hists->stats.total_non_filtered_period = 0;
+
+ /*
+ * recalculate total period using top-level entries only
+ * since lower level entries only see non-filtered entries
+ * but upper level entries have sum of both entries.
+ */
+ while (node) {
+ he = rb_entry(node, struct hist_entry, rb_node);
+ node = rb_next(node);
+
+ hists->stats.total_period += he->stat.period;
+ if (!he->filtered)
+ hists->stats.total_non_filtered_period += he->stat.period;
+ }
+}
+
static void hierarchy_insert_output_entry(struct rb_root *root,
struct hist_entry *he)
{
@@ -1518,11 +1543,6 @@ static void hists__hierarchy_output_resort(struct hists *hists,
continue;
}
- /* only update stat for leaf entries to avoid duplication */
- hists__inc_stats(hists, he);
- if (!he->filtered)
- hists__calc_col_len(hists, he);
-
if (!use_callchain)
continue;
@@ -1602,11 +1622,13 @@ static void output_resort(struct hists *hists, struct ui_progress *prog,
hists__reset_col_len(hists);
if (symbol_conf.report_hierarchy) {
- return hists__hierarchy_output_resort(hists, prog,
- &hists->entries_collapsed,
- &hists->entries,
- min_callchain_hits,
- use_callchain);
+ hists__hierarchy_output_resort(hists, prog,
+ &hists->entries_collapsed,
+ &hists->entries,
+ min_callchain_hits,
+ use_callchain);
+ hierarchy_recalc_total_periods(hists);
+ return;
}
if (sort__need_collapse)
@@ -1927,6 +1949,8 @@ static void hists__filter_hierarchy(struct hists *hists, int type, const void *a
}
}
+ hierarchy_recalc_total_periods(hists);
+
/*
* resort output after applying a new filter since filter in a lower
* hierarchy can change periods in a upper hierarchy.
[toc] | [prev] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-03-09 14:50 +0100 |
| Subject | [PATCH v2 1/7] perf tools: Fix hist_entry__filter() for hierarchy |
| Message-ID | <raMEX-9f-29@gated-at.bofh.it> |
| In reply to | #1354136 |
When hierarchy mode is enabled each output format is in a separate hpp
list. So when applying filter it should check all formats in the list.
Currently it only checks a single ->fmt field which was not set properly.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/sort.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 041f236379e0..c3c823377d39 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1602,16 +1602,30 @@ int hist_entry__filter(struct hist_entry *he, int type, const void *arg)
{
struct perf_hpp_fmt *fmt;
struct hpp_sort_entry *hse;
+ int ret = -1;
+ int r;
- fmt = he->fmt;
- if (fmt == NULL || !perf_hpp__is_sort_entry(fmt))
- return -1;
+ perf_hpp_list__for_each_format(he->hpp_list, fmt) {
+ if (!perf_hpp__is_sort_entry(fmt))
+ continue;
- hse = container_of(fmt, struct hpp_sort_entry, hpp);
- if (hse->se->se_filter == NULL)
- return -1;
+ hse = container_of(fmt, struct hpp_sort_entry, hpp);
+ if (hse->se->se_filter == NULL)
+ continue;
- return hse->se->se_filter(he, type, arg);
+ /*
+ * hist entry is filtered if any of sort key in the hpp list
+ * is applied. But it should skip non-matched filter types.
+ */
+ r = hse->se->se_filter(he, type, arg);
+ if (r >= 0) {
+ if (ret < 0)
+ ret = 0;
+ ret |= r;
+ }
+ }
+
+ return ret;
}
static int __sort_dimension__add_hpp_sort(struct sort_dimension *sd, int level)
--
2.7.2
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Namhyung Kim <tipbot@zytor.com> |
|---|---|
| Date | 2016-03-11 09:50 +0100 |
| Subject | [tip:perf/core] perf tools: Fix hist_entry__filter() for hierarchy |
| Message-ID | <rbqVI-3vt-5@gated-at.bofh.it> |
| In reply to | #1354141 |
Commit-ID: f4954cfb1cda4cf0abf36d23213c702e94666c3f
Gitweb: http://git.kernel.org/tip/f4954cfb1cda4cf0abf36d23213c702e94666c3f
Author: Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 9 Mar 2016 22:46:56 +0900
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Mar 2016 16:45:36 -0300
perf tools: Fix hist_entry__filter() for hierarchy
When hierarchy mode is enabled each output format is in a separate hpp
list. So when applying a filter it should check all formats in the
list. Currently it only checks a single ->fmt field which was not set
properly.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1457531222-18130-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/sort.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 59a101e..8a49a07 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1602,16 +1602,30 @@ int hist_entry__filter(struct hist_entry *he, int type, const void *arg)
{
struct perf_hpp_fmt *fmt;
struct hpp_sort_entry *hse;
+ int ret = -1;
+ int r;
- fmt = he->fmt;
- if (fmt == NULL || !perf_hpp__is_sort_entry(fmt))
- return -1;
+ perf_hpp_list__for_each_format(he->hpp_list, fmt) {
+ if (!perf_hpp__is_sort_entry(fmt))
+ continue;
- hse = container_of(fmt, struct hpp_sort_entry, hpp);
- if (hse->se->se_filter == NULL)
- return -1;
+ hse = container_of(fmt, struct hpp_sort_entry, hpp);
+ if (hse->se->se_filter == NULL)
+ continue;
- return hse->se->se_filter(he, type, arg);
+ /*
+ * hist entry is filtered if any of sort key in the hpp list
+ * is applied. But it should skip non-matched filter types.
+ */
+ r = hse->se->se_filter(he, type, arg);
+ if (r >= 0) {
+ if (ret < 0)
+ ret = 0;
+ ret |= r;
+ }
+ }
+
+ return ret;
}
static int __sort_dimension__add_hpp_sort(struct sort_dimension *sd,
[toc] | [prev] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-03-09 15:00 +0100 |
| Subject | [PATCH v2 3/7] perf tools: Fix command line filters in hierarchy mode |
| Message-ID | <raMOC-cx-3@gated-at.bofh.it> |
| In reply to | #1354136 |
When a command-line filter was applied in hierarchy mode, output was
broken especially when filtering on lower level. The higher level
entries didn't show up so it's hard to see the result.
Also it needs to handle multi sort keys in a single level of hierarchy.
Before:
$ perf report --hierarchy -s 'cpu,{dso,comm}' --comms swapper --stdio
...
# Overhead CPU / Shared Object+Command
# ........... ...........................
#
13.79% [kernel.vmlinux] swapper
31.71% 000
13.80% [kernel.vmlinux] swapper
0.43% [e1000e] swapper
11.89% [kernel.vmlinux] swapper
9.18% [kernel.vmlinux] swapper
After:
# Overhead CPU / Shared Object+Command
# ........... ...............................
#
33.09% 003
13.79% [kernel.vmlinux] swapper
31.71% 000
13.80% [kernel.vmlinux] swapper
0.43% [e1000e] swapper
21.90% 002
11.89% [kernel.vmlinux] swapper
13.30% 001
9.18% [kernel.vmlinux] swapper
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/hist.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 97 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 29da9e0d8db9..a98f9345f686 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1087,10 +1087,103 @@ int hist_entry__snprintf_alignment(struct hist_entry *he, struct perf_hpp *hpp,
*/
static void hists__apply_filters(struct hists *hists, struct hist_entry *he);
+static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *he,
+ enum hist_filter type);
+
+typedef bool (*fmt_chk_fn)(struct perf_hpp_fmt *fmt);
+
+static bool check_thread_entry(struct perf_hpp_fmt *fmt)
+{
+ return perf_hpp__is_thread_entry(fmt) || perf_hpp__is_comm_entry(fmt);
+}
+
+static void hist_entry__check_and_remove_filter(struct hist_entry *he,
+ enum hist_filter type,
+ fmt_chk_fn check)
+{
+ struct perf_hpp_fmt *fmt;
+ bool type_match = false;
+ struct hist_entry *parent = he->parent_he;
+
+ switch (type) {
+ case HIST_FILTER__THREAD:
+ if (symbol_conf.comm_list == NULL &&
+ symbol_conf.pid_list == NULL &&
+ symbol_conf.tid_list == NULL)
+ return;
+ break;
+ case HIST_FILTER__DSO:
+ if (symbol_conf.dso_list == NULL)
+ return;
+ break;
+ case HIST_FILTER__SYMBOL:
+ if (symbol_conf.sym_list == NULL)
+ return;
+ break;
+ case HIST_FILTER__PARENT:
+ case HIST_FILTER__GUEST:
+ case HIST_FILTER__HOST:
+ case HIST_FILTER__SOCKET:
+ default:
+ return;
+ }
+
+ /* if it's filtered by own fmt, it has to have filter bits */
+ perf_hpp_list__for_each_format(he->hpp_list, fmt) {
+ if (check(fmt)) {
+ type_match = true;
+ break;
+ }
+ }
+
+ if (type_match) {
+ /*
+ * If the filter is for current level entry, propagate
+ * filter marker to parents. The marker bit was
+ * already set by default so it only needs to clear
+ * non-filtered entries.
+ */
+ if (!(he->filtered & (1 << type))) {
+ while (parent) {
+ parent->filtered &= ~(1 << type);
+ parent = parent->parent_he;
+ }
+ }
+ } else {
+ /*
+ * If current entry doesn't have matching formats, set
+ * filter marker for upper level entries. it will be
+ * cleared if its lower level entries is not filtered.
+ *
+ * For lower-level entries, it inherits parent's
+ * filter bit so that lower level entries of a
+ * non-filtered entry won't set the filter marker.
+ */
+ if (parent == NULL)
+ he->filtered |= (1 << type);
+ else
+ he->filtered |= (parent->filtered & (1 << type));
+ }
+}
+
+static void hist_entry__apply_hierarchy_filters(struct hist_entry *he)
+{
+ hist_entry__check_and_remove_filter(he, HIST_FILTER__THREAD,
+ check_thread_entry);
+
+ hist_entry__check_and_remove_filter(he, HIST_FILTER__DSO,
+ perf_hpp__is_dso_entry);
+
+ hist_entry__check_and_remove_filter(he, HIST_FILTER__SYMBOL,
+ perf_hpp__is_sym_entry);
+
+ hists__apply_filters(he->hists, he);
+}
static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
struct rb_root *root,
struct hist_entry *he,
+ struct hist_entry *parent_he,
struct perf_hpp_list *hpp_list)
{
struct rb_node **p = &root->rb_node;
@@ -1125,11 +1218,13 @@ static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
if (new == NULL)
return NULL;
- hists__apply_filters(hists, new);
hists->nr_entries++;
/* save related format list for output */
new->hpp_list = hpp_list;
+ new->parent_he = parent_he;
+
+ hist_entry__apply_hierarchy_filters(new);
/* some fields are now passed to 'new' */
perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
@@ -1170,14 +1265,13 @@ static int hists__hierarchy_insert_entry(struct hists *hists,
continue;
/* insert copy of 'he' for each fmt into the hierarchy */
- new_he = hierarchy_insert_entry(hists, root, he, &node->hpp);
+ new_he = hierarchy_insert_entry(hists, root, he, parent, &node->hpp);
if (new_he == NULL) {
ret = -1;
break;
}
root = &new_he->hroot_in;
- new_he->parent_he = parent;
new_he->depth = depth++;
parent = new_he;
}
--
2.7.2
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Namhyung Kim <tipbot@zytor.com> |
|---|---|
| Date | 2016-03-11 09:50 +0100 |
| Subject | [tip:perf/core] perf tools: Fix command line filters in hierarchy mode |
| Message-ID | <rbqVI-3vt-9@gated-at.bofh.it> |
| In reply to | #1354145 |
Commit-ID: aec13a7ec78d9322a348fb26940097b0bdfef1bd
Gitweb: http://git.kernel.org/tip/aec13a7ec78d9322a348fb26940097b0bdfef1bd
Author: Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 9 Mar 2016 22:46:58 +0900
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Mar 2016 16:45:48 -0300
perf tools: Fix command line filters in hierarchy mode
When a command-line filter is applied in hierarchy mode, output is
broken especially when filtering on lower level. The higher level
entries doesn't show up so it's hard to see the results.
Also it needs to handle multi sort keys in a single hierarchy level.
Before:
$ perf report --hierarchy -s 'cpu,{dso,comm}' --comms swapper --stdio
...
# Overhead CPU / Shared Object+Command
# ........... ...........................
#
13.79% [kernel.vmlinux] swapper
31.71% 000
13.80% [kernel.vmlinux] swapper
0.43% [e1000e] swapper
11.89% [kernel.vmlinux] swapper
9.18% [kernel.vmlinux] swapper
After:
# Overhead CPU / Shared Object+Command
# ........... ...............................
#
33.09% 003
13.79% [kernel.vmlinux] swapper
31.71% 000
13.80% [kernel.vmlinux] swapper
0.43% [e1000e] swapper
21.90% 002
11.89% [kernel.vmlinux] swapper
13.30% 001
9.18% [kernel.vmlinux] swapper
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1457531222-18130-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/hist.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 97 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 29da9e0..a98f934 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1087,10 +1087,103 @@ int hist_entry__snprintf_alignment(struct hist_entry *he, struct perf_hpp *hpp,
*/
static void hists__apply_filters(struct hists *hists, struct hist_entry *he);
+static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *he,
+ enum hist_filter type);
+
+typedef bool (*fmt_chk_fn)(struct perf_hpp_fmt *fmt);
+
+static bool check_thread_entry(struct perf_hpp_fmt *fmt)
+{
+ return perf_hpp__is_thread_entry(fmt) || perf_hpp__is_comm_entry(fmt);
+}
+
+static void hist_entry__check_and_remove_filter(struct hist_entry *he,
+ enum hist_filter type,
+ fmt_chk_fn check)
+{
+ struct perf_hpp_fmt *fmt;
+ bool type_match = false;
+ struct hist_entry *parent = he->parent_he;
+
+ switch (type) {
+ case HIST_FILTER__THREAD:
+ if (symbol_conf.comm_list == NULL &&
+ symbol_conf.pid_list == NULL &&
+ symbol_conf.tid_list == NULL)
+ return;
+ break;
+ case HIST_FILTER__DSO:
+ if (symbol_conf.dso_list == NULL)
+ return;
+ break;
+ case HIST_FILTER__SYMBOL:
+ if (symbol_conf.sym_list == NULL)
+ return;
+ break;
+ case HIST_FILTER__PARENT:
+ case HIST_FILTER__GUEST:
+ case HIST_FILTER__HOST:
+ case HIST_FILTER__SOCKET:
+ default:
+ return;
+ }
+
+ /* if it's filtered by own fmt, it has to have filter bits */
+ perf_hpp_list__for_each_format(he->hpp_list, fmt) {
+ if (check(fmt)) {
+ type_match = true;
+ break;
+ }
+ }
+
+ if (type_match) {
+ /*
+ * If the filter is for current level entry, propagate
+ * filter marker to parents. The marker bit was
+ * already set by default so it only needs to clear
+ * non-filtered entries.
+ */
+ if (!(he->filtered & (1 << type))) {
+ while (parent) {
+ parent->filtered &= ~(1 << type);
+ parent = parent->parent_he;
+ }
+ }
+ } else {
+ /*
+ * If current entry doesn't have matching formats, set
+ * filter marker for upper level entries. it will be
+ * cleared if its lower level entries is not filtered.
+ *
+ * For lower-level entries, it inherits parent's
+ * filter bit so that lower level entries of a
+ * non-filtered entry won't set the filter marker.
+ */
+ if (parent == NULL)
+ he->filtered |= (1 << type);
+ else
+ he->filtered |= (parent->filtered & (1 << type));
+ }
+}
+
+static void hist_entry__apply_hierarchy_filters(struct hist_entry *he)
+{
+ hist_entry__check_and_remove_filter(he, HIST_FILTER__THREAD,
+ check_thread_entry);
+
+ hist_entry__check_and_remove_filter(he, HIST_FILTER__DSO,
+ perf_hpp__is_dso_entry);
+
+ hist_entry__check_and_remove_filter(he, HIST_FILTER__SYMBOL,
+ perf_hpp__is_sym_entry);
+
+ hists__apply_filters(he->hists, he);
+}
static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
struct rb_root *root,
struct hist_entry *he,
+ struct hist_entry *parent_he,
struct perf_hpp_list *hpp_list)
{
struct rb_node **p = &root->rb_node;
@@ -1125,11 +1218,13 @@ static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
if (new == NULL)
return NULL;
- hists__apply_filters(hists, new);
hists->nr_entries++;
/* save related format list for output */
new->hpp_list = hpp_list;
+ new->parent_he = parent_he;
+
+ hist_entry__apply_hierarchy_filters(new);
/* some fields are now passed to 'new' */
perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
@@ -1170,14 +1265,13 @@ static int hists__hierarchy_insert_entry(struct hists *hists,
continue;
/* insert copy of 'he' for each fmt into the hierarchy */
- new_he = hierarchy_insert_entry(hists, root, he, &node->hpp);
+ new_he = hierarchy_insert_entry(hists, root, he, parent, &node->hpp);
if (new_he == NULL) {
ret = -1;
break;
}
root = &new_he->hroot_in;
- new_he->parent_he = parent;
new_he->depth = depth++;
parent = new_he;
}
[toc] | [prev] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-03-09 15:00 +0100 |
| Subject | [PATCH v2 4/7] perf tools: Remove hist_entry->fmt field |
| Message-ID | <raMOC-cx-11@gated-at.bofh.it> |
| In reply to | #1354136 |
It's not used anymore and the output format is accessed by the hpp_list
pointer instead when hierarchy is enabled. Let's get rid of it.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/sort.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index ea1f722cffea..151afc1b6c2f 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -129,7 +129,6 @@ struct hist_entry {
void *raw_data;
u32 raw_size;
void *trace_output;
- struct perf_hpp_fmt *fmt;
struct perf_hpp_list *hpp_list;
struct hist_entry *parent_he;
union {
--
2.7.2
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Namhyung Kim <tipbot@zytor.com> |
|---|---|
| Date | 2016-03-11 10:00 +0100 |
| Subject | [tip:perf/core] perf tools: Remove hist_entry->fmt field |
| Message-ID | <rbr5q-3zy-49@gated-at.bofh.it> |
| In reply to | #1354148 |
Commit-ID: a515d8ff7085d5e9fde867f2048b8da36b95dc51
Gitweb: http://git.kernel.org/tip/a515d8ff7085d5e9fde867f2048b8da36b95dc51
Author: Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 9 Mar 2016 22:46:59 +0900
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Mar 2016 16:45:59 -0300
perf tools: Remove hist_entry->fmt field
It's not used anymore and the output format is accessed by the hpp_list
pointer instead when hierarchy is enabled. Let's get rid of it.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1457531222-18130-5-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/sort.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index ea1f722..151afc1 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -129,7 +129,6 @@ struct hist_entry {
void *raw_data;
u32 raw_size;
void *trace_output;
- struct perf_hpp_fmt *fmt;
struct perf_hpp_list *hpp_list;
struct hist_entry *parent_he;
union {
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-03-10 11:40 +0100 |
| Subject | Re: [PATCHSET 0/7] perf tools: Fix and cleanups for hierarchy mode (v2) |
| Message-ID | <rb6aB-5KR-3@gated-at.bofh.it> |
| In reply to | #1354136 |
On Wed, Mar 09, 2016 at 10:46:55PM +0900, Namhyung Kim wrote:
> Hello,
>
> These are filter-related fixes and cleanups in the new hierarchy mode.
> Currently perf misbehaves in hierarchy mode when filter is applied.
> For example, it sometimes misses some (upper level) entries in the
> output or shows incorrect (very large) overhead.
>
> * Changes in v2)
> - update hist_entry__filter() to check multiple filters properly
> - add missing runtime filter check when adding hierarchy entries
> - recalculate total period using top-level entries (Jiri)
> - add more comment
>
>
> Before:
>
> $ perf report --hierarchy -s 'cpu,{dso,comm}' --comms swapper --stdio
> ...
> # Overhead CPU / Shared Object+Command
> # ........... ...........................
> #
> 13.79% [kernel.vmlinux] swapper
> 31.71% 000
> 13.80% [kernel.vmlinux] swapper
> 0.43% [e1000e] swapper
> 11.89% [kernel.vmlinux] swapper
> 9.18% [kernel.vmlinux] swapper
>
> After:
>
> # Overhead CPU / Shared Object+Command
> # ........... ...............................
> #
> 33.09% 003
> 13.79% [kernel.vmlinux] swapper
> 31.71% 000
> 13.80% [kernel.vmlinux] swapper
> 0.43% [e1000e] swapper
> 21.90% 002
> 11.89% [kernel.vmlinux] swapper
> 13.30% 001
> 9.18% [kernel.vmlinux] swapper
>
>
> It's also available on 'perf/hierarchy-filter-v2' branch in my tree
>
> git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
> any feedback are welcome.
looks good to me, the 'F' toggle shows sane number for me now
Acked-and-tested-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web