All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/18] perf report: Add support for event group view (v6)
@ 2012-11-29  6:38 Namhyung Kim
  2012-11-29  6:38 ` Namhyung Kim
                   ` (19 more replies)
  0 siblings, 20 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen

Hi all,

This is my 6th iteration of event group view patchset.
For basic idea and usage example, please see my original post [1].

The largest change in this version is using 'pairs' list of hist_entry
instead of allocating group_stats for all group members.  But I needed
to allocate temporary arrays for internal purpose anyway.  If you have
a better solution please let me know.

Jiri, sorry for the delay.  I had to hunt down some nasty bugs in my
patches.  And I saw you submitted the multiplu diff series which will
conflict my changes.  I'll have a look at it soon, and I'd be glad if
you take a look at my changes too. :)

You can get this series via my tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git  perf/group-v6

Any comments are welcome, thanks,
Namhyung


v5 -> v6:
 * set ->leader alse for leader evsel (Arnaldo)
 * use hists__{match,link} (Arnaldo)

v4 -> v5:
 * rebase onto acme/perf/core

v3 -> v4:
 * patch 1-9 in previous post are merged.
 * add Jiri's Acked-by
 * add report.group config option

v2 -> v3:
 * drop patch 1 since it's merged into acme/perf/core
 * cherry-pick Jiri's hpp changes
 * add missing bswap_32 on reading nr_groups (Jiri)
 * remove perf_evlist__recalc_nr_groups() in favor of list_is_last (Jiri)

v1 -> v2:
 * save group relation to header (Jiri)

[1] https://lkml.org/lkml/2012/7/24/81


Namhyung Kim (18):
  perf evsel: Set leader evsel's ->leader to itself
  perf evsel: Convert to _is_group_leader method
  perf tools: Keep group information
  perf header: Add HEADER_GROUP_DESC feature
  perf tools: Fix typo on hist__entry_add_pair
  perf hists: Link hist entry pairs to leader
  perf hists: Exchange order of comparing items when collapsing hists
  perf hists: Add hists__{match,link}_collapsed
  perf symbol: Introduce symbol_conf.event_group
  perf report: Make another loop for linking group hists
  perf hists: Resort hist entries using group members for output
  perf ui/hist: Add support for event group view
  perf hist browser: Add support for event group view
  perf gtk/browser: Add support for event group view
  perf report: Bypass non-leader events when event group is enabled
  perf report: Show group description when event group is enabled
  perf report: Add --group option
  perf report: Add report.group config option

 tools/perf/Documentation/perf-report.txt |   3 +
 tools/perf/builtin-record.c              |   3 +
 tools/perf/builtin-report.c              |  47 +++-
 tools/perf/builtin-stat.c                |   2 +-
 tools/perf/tests/parse-events.c          |  20 +-
 tools/perf/ui/browsers/hists.c           | 237 ++++++++++++++++---
 tools/perf/ui/gtk/browser.c              |  99 ++++++--
 tools/perf/ui/hist.c                     | 375 ++++++++++++++++---------------
 tools/perf/ui/stdio/hist.c               |   2 +
 tools/perf/util/evlist.c                 |  12 +-
 tools/perf/util/evlist.h                 |   1 +
 tools/perf/util/evsel.c                  |  32 ++-
 tools/perf/util/evsel.h                  |  20 +-
 tools/perf/util/header.c                 | 153 +++++++++++++
 tools/perf/util/header.h                 |   2 +
 tools/perf/util/hist.c                   | 182 +++++++++++++--
 tools/perf/util/hist.h                   |   2 +
 tools/perf/util/parse-events.c           |   1 +
 tools/perf/util/parse-events.h           |   1 +
 tools/perf/util/parse-events.y           |  10 +
 tools/perf/util/sort.h                   |   2 +-
 tools/perf/util/symbol.c                 |   4 +
 tools/perf/util/symbol.h                 |   3 +-
 23 files changed, 934 insertions(+), 279 deletions(-)

-- 
1.7.11.7


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 00/18] perf report: Add support for event group view (v6)
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29 12:21   ` Jiri Olsa
  2012-11-29  6:38 ` [PATCH 01/18] perf evsel: Set leader evsel's ->leader to itself Namhyung Kim
                   ` (18 subsequent siblings)
  19 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen

Hi all,

This is my 6th iteration of event group view patchset.
For basic idea and usage example, please see my original post [1].

The largest change in this version is using 'pairs' list of hist_entry
instead of allocating group_stats for all group members.  But I needed
to allocate temporary arrays for internal purpose anyway.  If you have
a better solution please let me know.

Jiri, sorry for the delay, I had to hunt down some nasty bugs in my
patches.  And I saw you submitted the multiplu diff series which will
conflict my changes.  I'll have a look at it soon, and I'd be glad if
you take a look at my changes too. :)

You can get this series via my tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git  perf/group-v6

Any comments are welcome, thanks,
Namhyung


v5 -> v6:
 * set ->leader alse for leader evsel (Arnaldo)
 * use hists__{match,link} (Arnaldo)

v4 -> v5:
 * rebase onto acme/perf/core

v3 -> v4:
 * patch 1-9 in previous post are merged.
 * add Jiri's Acked-by
 * add report.group config option

v2 -> v3:
 * drop patch 1 since it's merged into acme/perf/core
 * cherry-pick Jiri's hpp changes
 * add missing bswap_32 on reading nr_groups (Jiri)
 * remove perf_evlist__recalc_nr_groups() in favor of list_is_last (Jiri)

v1 -> v2:
 * save group relation to header (Jiri)

[1] https://lkml.org/lkml/2012/7/24/81


Namhyung Kim (18):
  perf evsel: Set leader evsel's ->leader to itself
  perf evsel: Convert to _is_group_leader method
  perf tools: Keep group information
  perf header: Add HEADER_GROUP_DESC feature
  perf tools: Fix typo on hist__entry_add_pair
  perf hists: Link hist entry pairs to leader
  perf hists: Exchange order of comparing items when collapsing hists
  perf hists: Add hists__{match,link}_collapsed
  perf symbol: Introduce symbol_conf.event_group
  perf report: Make another loop for linking group hists
  perf hists: Resort hist entries using group members for output
  perf ui/hist: Add support for event group view
  perf hist browser: Add support for event group view
  perf gtk/browser: Add support for event group view
  perf report: Bypass non-leader events when event group is enabled
  perf report: Show group description when event group is enabled
  perf report: Add --group option
  perf report: Add report.group config option

 tools/perf/Documentation/perf-report.txt |   3 +
 tools/perf/builtin-record.c              |   3 +
 tools/perf/builtin-report.c              |  47 +++-
 tools/perf/builtin-stat.c                |   2 +-
 tools/perf/tests/parse-events.c          |  20 +-
 tools/perf/ui/browsers/hists.c           | 237 ++++++++++++++++---
 tools/perf/ui/gtk/browser.c              |  99 ++++++--
 tools/perf/ui/hist.c                     | 375 ++++++++++++++++---------------
 tools/perf/ui/stdio/hist.c               |   2 +
 tools/perf/util/evlist.c                 |  12 +-
 tools/perf/util/evlist.h                 |   1 +
 tools/perf/util/evsel.c                  |  32 ++-
 tools/perf/util/evsel.h                  |  20 +-
 tools/perf/util/header.c                 | 153 +++++++++++++
 tools/perf/util/header.h                 |   2 +
 tools/perf/util/hist.c                   | 182 +++++++++++++--
 tools/perf/util/hist.h                   |   2 +
 tools/perf/util/parse-events.c           |   1 +
 tools/perf/util/parse-events.h           |   1 +
 tools/perf/util/parse-events.y           |  10 +
 tools/perf/util/sort.h                   |   2 +-
 tools/perf/util/symbol.c                 |   4 +
 tools/perf/util/symbol.h                 |   3 +-
 23 files changed, 934 insertions(+), 279 deletions(-)

-- 
1.7.11.7


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 01/18] perf evsel: Set leader evsel's ->leader to itself
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
  2012-11-29  6:38 ` Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29 14:28   ` Jiri Olsa
  2013-01-24 18:48   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2012-11-29  6:38 ` [PATCH 02/18] perf evsel: Convert to _is_group_leader method Namhyung Kim
                   ` (17 subsequent siblings)
  19 siblings, 2 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Currently only non-leader members are set ->leader to the leader evsel
of the group and the leader has set NULL.  Thus it requires special
casing for leader evsels.  Set ->leader to itself will remove this.

Suggested-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evlist.c | 1 -
 tools/perf/util/evsel.c  | 1 +
 tools/perf/util/evsel.h  | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 705293489e3c..90db2a14e4bb 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -111,7 +111,6 @@ void __perf_evlist__set_leader(struct list_head *list)
 	struct perf_evsel *evsel, *leader;
 
 	leader = list_entry(list->next, struct perf_evsel, node);
-	leader->leader = NULL;
 
 	list_for_each_entry(evsel, list, node) {
 		if (evsel != leader)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1fb636c550a1..ceabe05c6b12 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -55,6 +55,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
 {
 	evsel->idx	   = idx;
 	evsel->attr	   = *attr;
+	evsel->leader	   = evsel;
 	INIT_LIST_HEAD(&evsel->node);
 	hists__init(&evsel->hists);
 	evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index a4c1dd4e149f..d504668e7ae5 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -228,6 +228,6 @@ static inline struct perf_evsel *perf_evsel__next(struct perf_evsel *evsel)
 
 static inline bool perf_evsel__is_group_member(const struct perf_evsel *evsel)
 {
-	return evsel->leader != NULL;
+	return evsel->leader != evsel;
 }
 #endif /* __PERF_EVSEL_H */
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 02/18] perf evsel: Convert to _is_group_leader method
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
  2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 01/18] perf evsel: Set leader evsel's ->leader to itself Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29 14:28   ` Jiri Olsa
  2013-01-24 18:49   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2012-11-29  6:38 ` [PATCH 03/18] perf tools: Keep group information Namhyung Kim
                   ` (16 subsequent siblings)
  19 siblings, 2 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Convert perf_evsel__is_group_member to perf_evsel__is_group_leader.
This is because the most usecases are using negative form to check
whether the given evsel is a leader or not and it's IMHO somewhat
ambiguous - leader also *is* a member of the group.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-stat.c       |  2 +-
 tools/perf/tests/parse-events.c | 20 ++++++++++----------
 tools/perf/util/evlist.c        |  4 ++--
 tools/perf/util/evsel.c         |  6 +++---
 tools/perf/util/evsel.h         |  4 ++--
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c247faca7127..c12655af2b88 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -153,7 +153,7 @@ retry:
 	}
 
 	if (!perf_target__has_task(&target) &&
-	    !perf_evsel__is_group_member(evsel)) {
+	    perf_evsel__is_group_leader(evsel)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 42a0c8cd3cd5..17f1cd656314 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -521,7 +521,7 @@ static int test__group1(struct perf_evlist *evlist)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* cycles:upp */
 	evsel = perf_evsel__next(evsel);
@@ -557,7 +557,7 @@ static int test__group2(struct perf_evlist *evlist)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* cache-references + :u modifier */
 	evsel = perf_evsel__next(evsel);
@@ -583,7 +583,7 @@ static int test__group2(struct perf_evlist *evlist)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	return 0;
 }
@@ -606,7 +606,7 @@ static int test__group3(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 	TEST_ASSERT_VAL("wrong group name",
 		!strcmp(leader->group_name, "group1"));
 
@@ -636,7 +636,7 @@ static int test__group3(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 	TEST_ASSERT_VAL("wrong group name",
 		!strcmp(leader->group_name, "group2"));
 
@@ -663,7 +663,7 @@ static int test__group3(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	return 0;
 }
@@ -687,7 +687,7 @@ static int test__group4(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", evsel->attr.precise_ip == 1);
 	TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* instructions:kp + p */
 	evsel = perf_evsel__next(evsel);
@@ -724,7 +724,7 @@ static int test__group5(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
 	TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* instructions + G */
 	evsel = perf_evsel__next(evsel);
@@ -751,7 +751,7 @@ static int test__group5(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
 	TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* instructions:G */
 	evsel = perf_evsel__next(evsel);
@@ -777,7 +777,7 @@ static int test__group5(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	return 0;
 }
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 90db2a14e4bb..d0e1e821fe85 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -221,7 +221,7 @@ void perf_evlist__disable(struct perf_evlist *evlist)
 
 	for (cpu = 0; cpu < evlist->cpus->nr; cpu++) {
 		list_for_each_entry(pos, &evlist->entries, node) {
-			if (perf_evsel__is_group_member(pos))
+			if (!perf_evsel__is_group_leader(pos))
 				continue;
 			for (thread = 0; thread < evlist->threads->nr; thread++)
 				ioctl(FD(pos, cpu, thread),
@@ -237,7 +237,7 @@ void perf_evlist__enable(struct perf_evlist *evlist)
 
 	for (cpu = 0; cpu < cpu_map__nr(evlist->cpus); cpu++) {
 		list_for_each_entry(pos, &evlist->entries, node) {
-			if (perf_evsel__is_group_member(pos))
+			if (!perf_evsel__is_group_leader(pos))
 				continue;
 			for (thread = 0; thread < evlist->threads->nr; thread++)
 				ioctl(FD(pos, cpu, thread),
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ceabe05c6b12..73e6085a7294 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -520,14 +520,14 @@ void perf_evsel__config(struct perf_evsel *evsel,
 	 * Disabling only independent events or group leaders,
 	 * keeping group members enabled.
 	 */
-	if (!perf_evsel__is_group_member(evsel))
+	if (perf_evsel__is_group_leader(evsel))
 		attr->disabled = 1;
 
 	/*
 	 * Setting enable_on_exec for independent events and
 	 * group leaders for traced executed by perf.
 	 */
-	if (perf_target__none(&opts->target) && !perf_evsel__is_group_member(evsel))
+	if (perf_target__none(&opts->target) && perf_evsel__is_group_leader(evsel))
 		attr->enable_on_exec = 1;
 }
 
@@ -708,7 +708,7 @@ static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
 	struct perf_evsel *leader = evsel->leader;
 	int fd;
 
-	if (!perf_evsel__is_group_member(evsel))
+	if (perf_evsel__is_group_leader(evsel))
 		return -1;
 
 	/*
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d504668e7ae5..46c8004ca56b 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -226,8 +226,8 @@ static inline struct perf_evsel *perf_evsel__next(struct perf_evsel *evsel)
 	return list_entry(evsel->node.next, struct perf_evsel, node);
 }
 
-static inline bool perf_evsel__is_group_member(const struct perf_evsel *evsel)
+static inline bool perf_evsel__is_group_leader(const struct perf_evsel *evsel)
 {
-	return evsel->leader != evsel;
+	return evsel->leader == evsel;
 }
 #endif /* __PERF_EVSEL_H */
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 03/18] perf tools: Keep group information
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (2 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 02/18] perf evsel: Convert to _is_group_leader method Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29 14:33   ` Jiri Olsa
  2012-11-29  6:38 ` [PATCH 04/18] perf header: Add HEADER_GROUP_DESC feature Namhyung Kim
                   ` (15 subsequent siblings)
  19 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Add a few of group-related field in struct perf_{evlist,evsel} so that
the group information in a evlist can be known easily.  It only counts
groups which have more than 1 members since leader-only groups are
treated as non-group events.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evlist.c       |  7 ++++++-
 tools/perf/util/evlist.h       |  1 +
 tools/perf/util/evsel.h        |  6 ++++++
 tools/perf/util/parse-events.c |  1 +
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/parse-events.y | 10 ++++++++++
 6 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d0e1e821fe85..a446b5b91530 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -111,6 +111,9 @@ void __perf_evlist__set_leader(struct list_head *list)
 	struct perf_evsel *evsel, *leader;
 
 	leader = list_entry(list->next, struct perf_evsel, node);
+	evsel = list_entry(list->prev, struct perf_evsel, node);
+
+	leader->nr_members = evsel->idx - leader->idx + 1;
 
 	list_for_each_entry(evsel, list, node) {
 		if (evsel != leader)
@@ -120,8 +123,10 @@ void __perf_evlist__set_leader(struct list_head *list)
 
 void perf_evlist__set_leader(struct perf_evlist *evlist)
 {
-	if (evlist->nr_entries)
+	if (evlist->nr_entries) {
+		evlist->nr_groups = evlist->nr_entries > 1 ? 1 : 0;
 		__perf_evlist__set_leader(&evlist->entries);
+	}
 }
 
 int perf_evlist__add_default(struct perf_evlist *evlist)
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 56003f779e60..fa76bf91c7e0 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -21,6 +21,7 @@ struct perf_evlist {
 	struct list_head entries;
 	struct hlist_head heads[PERF_EVLIST__HLIST_SIZE];
 	int		 nr_entries;
+	int		 nr_groups;
 	int		 nr_fds;
 	int		 nr_mmaps;
 	int		 mmap_len;
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 46c8004ca56b..887834ed0af1 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -73,6 +73,7 @@ struct perf_evsel {
 	bool 			needs_swap;
 	/* parse modifier helper */
 	int			exclude_GH;
+	int			nr_members;
 	struct perf_evsel	*leader;
 	char			*group_name;
 };
@@ -230,4 +231,9 @@ static inline bool perf_evsel__is_group_leader(const struct perf_evsel *evsel)
 {
 	return evsel->leader == evsel;
 }
+
+static inline int perf_evsel__group_idx(struct perf_evsel *evsel)
+{
+	return evsel->idx - evsel->leader->idx;
+}
 #endif /* __PERF_EVSEL_H */
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 020323af3364..25a556cf74ee 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -843,6 +843,7 @@ int parse_events(struct perf_evlist *evlist, const char *str,
 	if (!ret) {
 		int entries = data.idx - evlist->nr_entries;
 		perf_evlist__splice_list_tail(evlist, &data.list, entries);
+		evlist->nr_groups += data.nr_groups;
 		return 0;
 	}
 
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index f6399373d67d..5a42c6d7cc9a 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -65,6 +65,7 @@ struct parse_events__term {
 struct parse_events_data__events {
 	struct list_head list;
 	int idx;
+	int nr_groups;
 };
 
 struct parse_events_data__terms {
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 0f9914ae6bac..69cff676dcb1 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -122,6 +122,11 @@ group_def:
 PE_NAME '{' events '}'
 {
 	struct list_head *list = $3;
+	struct parse_events_data__events *data = _data;
+
+	/* Count groups only have more than 1 members */
+	if (!list_is_last(list->next, list))
+		data->nr_groups++;
 
 	parse_events__set_leader($1, list);
 	$$ = list;
@@ -130,6 +135,11 @@ PE_NAME '{' events '}'
 '{' events '}'
 {
 	struct list_head *list = $2;
+	struct parse_events_data__events *data = _data;
+
+	/* Count groups only have more than 1 members */
+	if (!list_is_last(list->next, list))
+		data->nr_groups++;
 
 	parse_events__set_leader(NULL, list);
 	$$ = list;
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 04/18] perf header: Add HEADER_GROUP_DESC feature
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (3 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 03/18] perf tools: Keep group information Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29 18:43   ` Arnaldo Carvalho de Melo
  2012-11-29  6:38 ` [PATCH 05/18] perf tools: Fix typo on hist__entry_add_pair Namhyung Kim
                   ` (14 subsequent siblings)
  19 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Save group relationship information so that it can be restored when
perf report is running.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-record.c |   3 +
 tools/perf/util/header.c    | 153 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/header.h    |   2 +
 3 files changed, 158 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f3151d3c70ce..d50f0dcfe1c1 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -592,6 +592,9 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 		goto out_delete_session;
 	}
 
+	if (!evsel_list->nr_groups)
+		perf_header__clear_feat(&session->header, HEADER_GROUP_DESC);
+
 	/*
 	 * perf_session__delete(session) will be called at perf_record__exit()
 	 */
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 195a47a8f052..6ae8185842f2 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1073,6 +1073,41 @@ static int write_pmu_mappings(int fd, struct perf_header *h __maybe_unused,
 }
 
 /*
+ * File format:
+ *
+ * struct group_descs {
+ *	u32	nr_groups;
+ *	struct group_desc {
+ *		char	name[];
+ *		u32	leader_idx;
+ *		u32	nr_members;
+ *	}[nr_groups];
+ * };
+ */
+static int write_group_desc(int fd, struct perf_header *h __maybe_unused,
+			    struct perf_evlist *evlist)
+{
+	u32 nr_groups = evlist->nr_groups;
+	struct perf_evsel *evsel;
+
+	do_write(fd, &nr_groups, sizeof(nr_groups));
+
+	list_for_each_entry(evsel, &evlist->entries, node) {
+		if (perf_evsel__is_group_leader(evsel) &&
+		    evsel->nr_members > 1) {
+			const char *name = evsel->group_name ?: "{anon_group}";
+			u32 leader_idx = evsel->idx;
+			u32 nr_members = evsel->nr_members;
+
+			do_write_string(fd, name);
+			do_write(fd, &leader_idx, sizeof(leader_idx));
+			do_write(fd, &nr_members, sizeof(nr_members));
+		}
+	}
+	return 0;
+}
+
+/*
  * default get_cpuid(): nothing gets recorded
  * actual implementation must be in arch/$(ARCH)/util/header.c
  */
@@ -1433,6 +1468,31 @@ error:
 	fprintf(fp, "# pmu mappings: unable to read\n");
 }
 
+static void print_group_desc(struct perf_header *ph, int fd __maybe_unused,
+			     FILE *fp)
+{
+	struct perf_session *session;
+	struct perf_evsel *evsel;
+	u32 nr = 0;
+
+	session = container_of(ph, struct perf_session, header);
+
+	list_for_each_entry(evsel, &session->evlist->entries, node) {
+		if (perf_evsel__is_group_leader(evsel) &&
+		    evsel->nr_members > 1) {
+			fprintf(fp, "# group: %s{%s", evsel->group_name ?: "",
+				perf_evsel__name(evsel));
+
+			nr = evsel->nr_members - 1;
+		} else if (nr) {
+			fprintf(fp, ",%s", perf_evsel__name(evsel));
+
+			if (--nr == 0)
+				fprintf(fp, "}\n");
+		}
+	}
+}
+
 static int __event_process_build_id(struct build_id_event *bev,
 				    char *filename,
 				    struct perf_session *session)
@@ -1947,6 +2007,98 @@ error:
 	return -1;
 }
 
+static int process_group_desc(struct perf_file_section *section __maybe_unused,
+			      struct perf_header *ph, int fd,
+			      void *data __maybe_unused)
+{
+	size_t ret = -1;
+	u32 i, nr, nr_groups;
+	struct perf_session *session;
+	struct perf_evsel *evsel, *leader = NULL;
+	struct group_desc {
+		char *name;
+		u32 leader_idx;
+		u32 nr_members;
+	} *desc;
+
+	if (read(fd, &nr_groups, sizeof(nr_groups)) != sizeof(nr_groups))
+		return -1;
+
+	if (ph->needs_swap)
+		nr_groups = bswap_32(nr_groups);
+
+	ph->env.nr_groups = nr_groups;
+	if (!nr_groups) {
+		pr_debug("group desc not available\n");
+		return 0;
+	}
+
+	desc = calloc(nr_groups, sizeof(*desc));
+	if (!desc)
+		return -1;
+
+	for (i = 0; i < nr_groups; i++) {
+		desc[i].name = do_read_string(fd, ph);
+		if (!desc[i].name)
+			goto out_free;
+
+		if (read(fd, &desc[i].leader_idx, sizeof(u32)) != sizeof(u32))
+			goto out_free;
+
+		if (read(fd, &desc[i].nr_members, sizeof(u32)) != sizeof(u32))
+			goto out_free;
+
+		if (ph->needs_swap) {
+			desc[i].leader_idx = bswap_32(desc[i].leader_idx);
+			desc[i].nr_members = bswap_32(desc[i].nr_members);
+		}
+	}
+
+	/*
+	 * Rebuild group relationship based on the group_desc
+	 */
+	session = container_of(ph, struct perf_session, header);
+	session->evlist->nr_groups = nr_groups;
+
+	i = nr = 0;
+	list_for_each_entry(evsel, &session->evlist->entries, node) {
+		if (evsel->idx == (int) desc[i].leader_idx) {
+			evsel->leader = evsel;
+			/* {anon_group} is a dummy name */
+			if (strcmp(desc[i].name, "{anon_group}"))
+				evsel->group_name = desc[i].name;
+			evsel->nr_members = desc[i].nr_members;
+
+			if (i >= nr_groups || nr > 0) {
+				pr_debug("invalid group desc\n");
+				goto out_free;
+			}
+
+			leader = evsel;
+			nr = evsel->nr_members - 1;
+			i++;
+		} else if (nr) {
+			/* This is a group member */
+			evsel->leader = leader;
+
+			nr--;
+		}
+	}
+
+	if (i != nr_groups || nr != 0) {
+		pr_debug("invalid group desc\n");
+		goto out_free;
+	}
+
+	ret = 0;
+out_free:
+	while ((int) --i >= 0)
+		free(desc[i].name);
+	free(desc);
+
+	return ret;
+}
+
 struct feature_ops {
 	int (*write)(int fd, struct perf_header *h, struct perf_evlist *evlist);
 	void (*print)(struct perf_header *h, int fd, FILE *fp);
@@ -1986,6 +2138,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
 	FEAT_OPF(HEADER_NUMA_TOPOLOGY,	numa_topology),
 	FEAT_OPA(HEADER_BRANCH_STACK,	branch_stack),
 	FEAT_OPP(HEADER_PMU_MAPPINGS,	pmu_mappings),
+	FEAT_OPP(HEADER_GROUP_DESC,	group_desc),
 };
 
 struct header_print_data {
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 5f1cd6884f37..e3e7fb490310 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -29,6 +29,7 @@ enum {
 	HEADER_NUMA_TOPOLOGY,
 	HEADER_BRANCH_STACK,
 	HEADER_PMU_MAPPINGS,
+	HEADER_GROUP_DESC,
 	HEADER_LAST_FEATURE,
 	HEADER_FEAT_BITS	= 256,
 };
@@ -79,6 +80,7 @@ struct perf_session_env {
 	char			*numa_nodes;
 	int			nr_pmu_mappings;
 	char			*pmu_mappings;
+	int			nr_groups;
 };
 
 struct perf_header {
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 05/18] perf tools: Fix typo on hist__entry_add_pair
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (4 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 04/18] perf header: Add HEADER_GROUP_DESC feature Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29 14:33   ` Jiri Olsa
  2012-11-29  6:38 ` [PATCH 06/18] perf hists: Link hist entry pairs to leader Namhyung Kim
                   ` (13 subsequent siblings)
  19 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Fix a misplaced underscore.  In this case, 'hist_entry' is the name of
data structure and we usually put double underscores between data
structure and actual function name.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 4 ++--
 tools/perf/util/sort.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index cb17e2a8c6ed..d2bc05cd1708 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -785,7 +785,7 @@ void hists__match(struct hists *leader, struct hists *other)
 		pair = hists__find_entry(other, pos);
 
 		if (pair)
-			hist__entry_add_pair(pos, pair);
+			hist_entry__add_pair(pos, pair);
 	}
 }
 
@@ -806,7 +806,7 @@ int hists__link(struct hists *leader, struct hists *other)
 			pair = hists__add_dummy_entry(leader, pos);
 			if (pair == NULL)
 				return -1;
-			hist__entry_add_pair(pair, pos);
+			hist_entry__add_pair(pair, pos);
 		}
 	}
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index b4e8c3ba559d..91ae2744e7d5 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -118,7 +118,7 @@ static inline struct hist_entry *hist_entry__next_pair(struct hist_entry *he)
 	return NULL;
 }
 
-static inline void hist__entry_add_pair(struct hist_entry *he,
+static inline void hist_entry__add_pair(struct hist_entry *he,
 					struct hist_entry *pair)
 {
 	list_add_tail(&he->pairs.head, &pair->pairs.node);
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 06/18] perf hists: Link hist entry pairs to leader
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (5 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 05/18] perf tools: Fix typo on hist__entry_add_pair Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2013-01-24 18:47   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2012-11-29  6:38 ` [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists Namhyung Kim
                   ` (12 subsequent siblings)
  19 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Current hists__match/link() link a leader to its pair, so if multiple
pairs were linked, the leader will lose pointer to previous pairs
since it was overwritten.  Fix it by making leader the list head.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index d2bc05cd1708..82df1b26f0d4 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -785,7 +785,7 @@ void hists__match(struct hists *leader, struct hists *other)
 		pair = hists__find_entry(other, pos);
 
 		if (pair)
-			hist_entry__add_pair(pos, pair);
+			hist_entry__add_pair(pair, pos);
 	}
 }
 
@@ -806,7 +806,7 @@ int hists__link(struct hists *leader, struct hists *other)
 			pair = hists__add_dummy_entry(leader, pos);
 			if (pair == NULL)
 				return -1;
-			hist_entry__add_pair(pair, pos);
+			hist_entry__add_pair(pos, pair);
 		}
 	}
 
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (6 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 06/18] perf hists: Link hist entry pairs to leader Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29 18:52   ` Arnaldo Carvalho de Melo
  2012-11-29  6:38 ` [PATCH 08/18] perf hists: Add hists__{match,link}_collapsed Namhyung Kim
                   ` (11 subsequent siblings)
  19 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

When comparing entries for collapsing put the given entry first, and
then the iterated entry.  This is for the sake of consistency and will
be required by the event group report.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 82df1b26f0d4..161c35e7ed0e 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -433,7 +433,7 @@ static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 		parent = *p;
 		iter = rb_entry(parent, struct hist_entry, rb_node_in);
 
-		cmp = hist_entry__collapse(iter, he);
+		cmp = hist_entry__collapse(he, iter);
 
 		if (!cmp) {
 			he_stat__add_stat(&iter->stat, &he->stat);
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 08/18] perf hists: Add hists__{match,link}_collapsed
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (7 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 09/18] perf symbol: Introduce symbol_conf.event_group Namhyung Kim
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

The current hists__match/link functions are worked on the output tree
(hists->entries).  However for event group view it needs to be
matched/linked before the final resort.  Thus add new helpers for
doing it on the collapsed tree.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 117 +++++++++++++++++++++++++++++++++++++++++--------
 tools/perf/util/hist.h |   2 +
 2 files changed, 101 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 161c35e7ed0e..2af8ec6e8e26 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -717,19 +717,72 @@ void hists__inc_nr_events(struct hists *hists, u32 type)
 	++hists->stats.nr_events[type];
 }
 
+struct rb_hist_fns {
+	struct rb_root * (*get_rb_root)(struct hists *);
+	struct rb_node * (*get_rb_node)(struct hist_entry *);
+	struct hist_entry * (*get_hist_entry)(struct rb_node *);
+	int64_t (*cmp_hist_entry)(struct hist_entry *, struct hist_entry *);
+};
+
+static struct rb_root *get_output_rb_root(struct hists *hists)
+{
+	return &hists->entries;
+}
+
+static struct rb_node *get_output_rb_node(struct hist_entry *he)
+{
+	return &he->rb_node;
+}
+
+static struct hist_entry *get_output_hist_entry(struct rb_node *node)
+{
+	return rb_entry(node, struct hist_entry, rb_node);
+}
+
+static const struct rb_hist_fns output_fns = {
+	.get_rb_root		= get_output_rb_root,
+	.get_rb_node		= get_output_rb_node,
+	.get_hist_entry		= get_output_hist_entry,
+	.cmp_hist_entry		= hist_entry__cmp,
+};
+
+static struct rb_root *get_collapsed_rb_root(struct hists *hists)
+{
+	return &hists->entries_collapsed;
+}
+
+static struct rb_node *get_collapsed_rb_node(struct hist_entry *he)
+{
+	return &he->rb_node_in;
+}
+
+static struct hist_entry *get_collapsed_hist_entry(struct rb_node *node)
+{
+	return rb_entry(node, struct hist_entry, rb_node_in);
+}
+
+static const struct rb_hist_fns collapsed_fns = {
+	.get_rb_root		= get_collapsed_rb_root,
+	.get_rb_node		= get_collapsed_rb_node,
+	.get_hist_entry		= get_collapsed_hist_entry,
+	.cmp_hist_entry		= hist_entry__collapse,
+};
+
 static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
-						 struct hist_entry *pair)
+						 struct hist_entry *pair,
+						 const struct rb_hist_fns *fns)
 {
-	struct rb_node **p = &hists->entries.rb_node;
+	struct rb_root *root = fns->get_rb_root(hists);
+	struct rb_node **p = &root->rb_node;
 	struct rb_node *parent = NULL;
 	struct hist_entry *he;
 	int cmp;
 
 	while (*p != NULL) {
 		parent = *p;
-		he = rb_entry(parent, struct hist_entry, rb_node);
+		he = fns->get_hist_entry(parent);
 
-		cmp = hist_entry__cmp(pair, he);
+		cmp = fns->cmp_hist_entry(pair, he);
 
 		if (!cmp)
 			goto out;
@@ -742,10 +795,12 @@ static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
 
 	he = hist_entry__new(pair);
 	if (he) {
+		struct rb_node *n = fns->get_rb_node(he);
+
 		memset(&he->stat, 0, sizeof(he->stat));
 		he->hists = hists;
-		rb_link_node(&he->rb_node, parent, p);
-		rb_insert_color(&he->rb_node, &hists->entries);
+		rb_link_node(n, parent, p);
+		rb_insert_color(n, root);
 		hists__inc_nr_entries(hists, he);
 	}
 out:
@@ -753,13 +808,15 @@ out:
 }
 
 static struct hist_entry *hists__find_entry(struct hists *hists,
-					    struct hist_entry *he)
+					    struct hist_entry *he,
+					    const struct rb_hist_fns *fns)
 {
-	struct rb_node *n = hists->entries.rb_node;
+	struct rb_root *root = fns->get_rb_root(hists);
+	struct rb_node *n = root->rb_node;
 
 	while (n) {
-		struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
-		int64_t cmp = hist_entry__cmp(he, iter);
+		struct hist_entry *iter = fns->get_hist_entry(n);
+		int64_t cmp = fns->cmp_hist_entry(he, iter);
 
 		if (cmp < 0)
 			n = n->rb_left;
@@ -775,35 +832,49 @@ static struct hist_entry *hists__find_entry(struct hists *hists,
 /*
  * Look for pairs to link to the leader buckets (hist_entries):
  */
-void hists__match(struct hists *leader, struct hists *other)
+static void __hists__match(struct hists *leader, struct hists *other,
+			   const struct rb_hist_fns *fns)
 {
+	struct rb_root *root = fns->get_rb_root(leader);
 	struct rb_node *nd;
 	struct hist_entry *pos, *pair;
 
-	for (nd = rb_first(&leader->entries); nd; nd = rb_next(nd)) {
-		pos  = rb_entry(nd, struct hist_entry, rb_node);
-		pair = hists__find_entry(other, pos);
+	for (nd = rb_first(root); nd; nd = rb_next(nd)) {
+		pos  = fns->get_hist_entry(nd);
+		pair = hists__find_entry(other, pos, fns);
 
 		if (pair)
 			hist_entry__add_pair(pair, pos);
 	}
 }
 
+void hists__match(struct hists *leader, struct hists *other)
+{
+	__hists__match(leader, other, &output_fns);
+}
+
+void hists__match_collapsed(struct hists *leader, struct hists *other)
+{
+	__hists__match(leader, other, &collapsed_fns);
+}
+
 /*
  * Look for entries in the other hists that are not present in the leader, if
  * we find them, just add a dummy entry on the leader hists, with period=0,
  * nr_events=0, to serve as the list header.
  */
-int hists__link(struct hists *leader, struct hists *other)
+static int __hists__link(struct hists *leader, struct hists *other,
+			 const struct rb_hist_fns *fns)
 {
+	struct rb_root *root = fns->get_rb_root(other);
 	struct rb_node *nd;
 	struct hist_entry *pos, *pair;
 
-	for (nd = rb_first(&other->entries); nd; nd = rb_next(nd)) {
-		pos = rb_entry(nd, struct hist_entry, rb_node);
+	for (nd = rb_first(root); nd; nd = rb_next(nd)) {
+		pos = fns->get_hist_entry(nd);
 
 		if (!hist_entry__has_pairs(pos)) {
-			pair = hists__add_dummy_entry(leader, pos);
+			pair = hists__add_dummy_entry(leader, pos, fns);
 			if (pair == NULL)
 				return -1;
 			hist_entry__add_pair(pos, pair);
@@ -812,3 +883,13 @@ int hists__link(struct hists *leader, struct hists *other)
 
 	return 0;
 }
+
+int hists__link(struct hists *leader, struct hists *other)
+{
+	return __hists__link(leader, other, &output_fns);
+}
+
+int hists__link_collapsed(struct hists *leader, struct hists *other)
+{
+	return __hists__link(leader, other, &collapsed_fns);
+}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 8b091a51e4a2..43ee5e4fc33e 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -116,7 +116,9 @@ void hists__reset_col_len(struct hists *hists);
 void hists__calc_col_len(struct hists *hists, struct hist_entry *he);
 
 void hists__match(struct hists *leader, struct hists *other);
+void hists__match_collapsed(struct hists *leader, struct hists *other);
 int hists__link(struct hists *leader, struct hists *other);
+int hists__link_collapsed(struct hists *leader, struct hists *other);
 
 struct perf_hpp {
 	char *buf;
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 09/18] perf symbol: Introduce symbol_conf.event_group
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (8 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 08/18] perf hists: Add hists__{match,link}_collapsed Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 10/18] perf report: Make another loop for linking group hists Namhyung Kim
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

The event_group field is for enabling event group view on perf report
and other commands.  It requires collapsing hist entries since every
member in a group needs to be linked before final output resorting.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 4 ++++
 tools/perf/util/symbol.h | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 295f8d4feedf..a64b0bf77f32 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -15,6 +15,7 @@
 #include "machine.h"
 #include "symbol.h"
 #include "strlist.h"
+#include "sort.h"
 
 #include <elf.h>
 #include <limits.h>
@@ -1650,6 +1651,9 @@ int symbol__init(void)
 
 	symbol_conf.kptr_restrict = symbol__read_kptr_restrict();
 
+	if (symbol_conf.event_group)
+		sort__need_collapse = 1;
+
 	symbol_conf.initialized = true;
 	return 0;
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index de68f98b236d..8bef1452675f 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -96,7 +96,8 @@ struct symbol_conf {
 			initialized,
 			kptr_restrict,
 			annotate_asm_raw,
-			annotate_src;
+			annotate_src,
+			event_group;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 10/18] perf report: Make another loop for linking group hists
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (9 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 09/18] perf symbol: Introduce symbol_conf.event_group Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 11/18] perf hists: Resort hist entries using group members for output Namhyung Kim
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Now the event grouping viewing requires linking all member hists in a
group to the leader's.  Thus hists__output_resort should be called
after linking all events in evlist.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index fc251005dd3d..13a2cd20a1cd 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -416,8 +416,16 @@ static int __cmd_report(struct perf_report *rep)
 			hists->symbol_filter_str = rep->symbol_filter_str;
 
 		hists__collapse_resort(hists);
-		hists__output_resort(hists);
 		nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE];
+
+		/* Non-group events are considered as leader */
+		if (symbol_conf.event_group &&
+		    !perf_evsel__is_group_leader(pos)) {
+			struct hists *leader_hists = &pos->leader->hists;
+
+			hists__match_collapsed(leader_hists, hists);
+			hists__link_collapsed(leader_hists, hists);
+		}
 	}
 
 	if (nr_samples == 0) {
@@ -425,6 +433,9 @@ static int __cmd_report(struct perf_report *rep)
 		goto out_delete;
 	}
 
+	list_for_each_entry(pos, &session->evlist->entries, node)
+		hists__output_resort(&pos->hists);
+
 	if (use_browser > 0) {
 		if (use_browser == 1) {
 			perf_evlist__tui_browse_hists(session->evlist, help,
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 11/18] perf hists: Resort hist entries using group members for output
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (10 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 10/18] perf report: Make another loop for linking group hists Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 12/18] perf ui/hist: Add support for event group view Namhyung Kim
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

When event group is enabled, sorting hist entries on periods for
output should consider groups members' period also.  To do that, build
period table using link/pair information and compare the table.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.h |  2 ++
 tools/perf/util/hist.c  | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 887834ed0af1..54a8efbd8dcb 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -78,6 +78,8 @@ struct perf_evsel {
 	char			*group_name;
 };
 
+#define hists_to_evsel(h) container_of(h, struct perf_evsel, hists)
+
 struct cpu_map;
 struct thread_map;
 struct perf_evlist;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 2af8ec6e8e26..be03277732e7 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -4,6 +4,7 @@
 #include "hist.h"
 #include "session.h"
 #include "sort.h"
+#include "evsel.h"
 #include <math.h>
 
 static bool hists__filter_entry_by_dso(struct hists *hists,
@@ -523,6 +524,62 @@ void hists__collapse_resort_threaded(struct hists *hists)
  * reverse the map, sort on period.
  */
 
+static int period_cmp(u64 period_a, u64 period_b)
+{
+	if (period_a > period_b)
+		return 1;
+	if (period_a < period_b)
+		return -1;
+	return 0;
+}
+
+static int hist_entry__sort_on_period(struct hist_entry *a,
+				      struct hist_entry *b)
+{
+	int ret;
+	int i, nr_members;
+	struct perf_evsel *evsel;
+	struct hist_entry *pair;
+	u64 *periods_a, *periods_b;
+
+	ret = period_cmp(a->stat.period, b->stat.period);
+	if (ret || !symbol_conf.event_group)
+		return ret;
+
+	evsel = hists_to_evsel(a->hists);
+	nr_members = evsel->nr_members;
+	if (nr_members <= 1)
+		return ret;
+
+	periods_a = zalloc(sizeof(periods_a) * nr_members);
+	periods_b = zalloc(sizeof(periods_b) * nr_members);
+
+	if (!periods_a || !periods_b)
+		goto out;
+
+	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
+		evsel = hists_to_evsel(pair->hists);
+		periods_a[perf_evsel__group_idx(evsel)] = pair->stat.period;
+	}
+
+	list_for_each_entry(pair, &b->pairs.head, pairs.node) {
+		evsel = hists_to_evsel(pair->hists);
+		periods_b[perf_evsel__group_idx(evsel)] = pair->stat.period;
+	}
+
+	for (i = 1; i < nr_members; i++) {
+		ret = period_cmp(periods_a[i], periods_b[i]);
+		if (ret)
+			break;
+	}
+
+out:
+	free(periods_a);
+	free(periods_b);
+
+	return ret;
+}
+
 static void __hists__insert_output_entry(struct rb_root *entries,
 					 struct hist_entry *he,
 					 u64 min_callchain_hits)
@@ -539,7 +596,7 @@ static void __hists__insert_output_entry(struct rb_root *entries,
 		parent = *p;
 		iter = rb_entry(parent, struct hist_entry, rb_node);
 
-		if (he->stat.period > iter->stat.period)
+		if (hist_entry__sort_on_period(he, iter) > 0)
 			p = &(*p)->rb_left;
 		else
 			p = &(*p)->rb_right;
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (11 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 11/18] perf hists: Resort hist entries using group members for output Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
       [not found]   ` <20121130132943.GC1080@krava.brq.redhat.com>
  2012-11-29  6:38 ` [PATCH 13/18] perf hist browser: " Namhyung Kim
                   ` (6 subsequent siblings)
  19 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Show group members' overhead also when showing the leader's if event
group is enabled.  Use macro for defining hpp functions which looks
almost identical.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c       | 370 +++++++++++++++++++++++----------------------
 tools/perf/ui/stdio/hist.c |   2 +
 2 files changed, 191 insertions(+), 181 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index aa84130024d5..b42bd15af3f5 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -3,152 +3,198 @@
 #include "../util/hist.h"
 #include "../util/util.h"
 #include "../util/sort.h"
-
+#include "../util/evsel.h"
 
 /* hist period print (hpp) functions */
-static int hpp__header_overhead(struct perf_hpp *hpp)
-{
-	return scnprintf(hpp->buf, hpp->size, "Overhead");
-}
-
-static int hpp__width_overhead(struct perf_hpp *hpp __maybe_unused)
-{
-	return 8;
-}
-
-static int hpp__color_overhead(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period / hists->stats.total_period;
-
-	return percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent);
-}
-
-static int hpp__entry_overhead(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period / hists->stats.total_period;
-	const char *fmt = symbol_conf.field_sep ? "%.2f" : " %6.2f%%";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, percent);
-}
-
-static int hpp__header_overhead_sys(struct perf_hpp *hpp)
-{
-	const char *fmt = symbol_conf.field_sep ? "%s" : "%7s";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, "sys");
-}
-
-static int hpp__width_overhead_sys(struct perf_hpp *hpp __maybe_unused)
-{
-	return 7;
-}
-
-static int hpp__color_overhead_sys(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_sys / hists->stats.total_period;
-
-	return percent_color_snprintf(hpp->buf, hpp->size, "%6.2f%%", percent);
-}
-
-static int hpp__entry_overhead_sys(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_sys / hists->stats.total_period;
-	const char *fmt = symbol_conf.field_sep ? "%.2f" : "%6.2f%%";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, percent);
-}
-
-static int hpp__header_overhead_us(struct perf_hpp *hpp)
-{
-	const char *fmt = symbol_conf.field_sep ? "%s" : "%7s";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, "user");
-}
-
-static int hpp__width_overhead_us(struct perf_hpp *hpp __maybe_unused)
-{
-	return 7;
-}
-
-static int hpp__color_overhead_us(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_us / hists->stats.total_period;
-
-	return percent_color_snprintf(hpp->buf, hpp->size, "%6.2f%%", percent);
-}
-
-static int hpp__entry_overhead_us(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_us / hists->stats.total_period;
-	const char *fmt = symbol_conf.field_sep ? "%.2f" : "%6.2f%%";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, percent);
-}
-
-static int hpp__header_overhead_guest_sys(struct perf_hpp *hpp)
-{
-	return scnprintf(hpp->buf, hpp->size, "guest sys");
-}
-
-static int hpp__width_overhead_guest_sys(struct perf_hpp *hpp __maybe_unused)
-{
-	return 9;
-}
-
-static int hpp__color_overhead_guest_sys(struct perf_hpp *hpp,
-					 struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_guest_sys / hists->stats.total_period;
-
-	return percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%% ", percent);
-}
-
-static int hpp__entry_overhead_guest_sys(struct perf_hpp *hpp,
-					 struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_guest_sys / hists->stats.total_period;
-	const char *fmt = symbol_conf.field_sep ? "%.2f" : " %6.2f%% ";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, percent);
-}
-
-static int hpp__header_overhead_guest_us(struct perf_hpp *hpp)
-{
-	return scnprintf(hpp->buf, hpp->size, "guest usr");
-}
-
-static int hpp__width_overhead_guest_us(struct perf_hpp *hpp __maybe_unused)
-{
-	return 9;
-}
-
-static int hpp__color_overhead_guest_us(struct perf_hpp *hpp,
-					struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_guest_us / hists->stats.total_period;
-
-	return percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%% ", percent);
-}
-
-static int hpp__entry_overhead_guest_us(struct perf_hpp *hpp,
-					struct hist_entry *he)
-{
-	struct hists *hists = he->hists;
-	double percent = 100.0 * he->stat.period_guest_us / hists->stats.total_period;
-	const char *fmt = symbol_conf.field_sep ? "%.2f" : " %6.2f%% ";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, percent);
-}
 
+#define __HPP_HEADER_FN(_type, _str, _min_width, _unit_width) 		\
+static int hpp__header_##_type(struct perf_hpp *hpp)			\
+{									\
+	int len = _min_width;						\
+									\
+	if (symbol_conf.event_group) {					\
+		struct perf_evsel *evsel = hpp->ptr;			\
+									\
+		len = max(len, evsel->nr_members * _unit_width);	\
+	}								\
+	return scnprintf(hpp->buf, hpp->size, "%*s", len, _str);	\
+}
+
+#define __HPP_WIDTH_FN(_type, _min_width, _unit_width) 			\
+static int hpp__width_##_type(struct perf_hpp *hpp)			\
+{									\
+	int len = _min_width;						\
+									\
+	if (symbol_conf.event_group) {					\
+		struct perf_evsel *evsel = hpp->ptr;			\
+									\
+		len = max(len, evsel->nr_members * _unit_width);	\
+	}								\
+	return len;							\
+}
+
+#define __HPP_COLOR_PERCENT_FN(_type, _field)					\
+static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he) 	\
+{										\
+	int ret;								\
+	double percent = 0.0;							\
+	struct hists *hists = he->hists;					\
+										\
+	if (hists->stats.total_period)						\
+		percent = 100.0 * he->stat._field / hists->stats.total_period; 	\
+										\
+	ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent);	\
+										\
+	if (symbol_conf.event_group) {						\
+		int i;								\
+		struct perf_evsel *evsel = hists_to_evsel(hists);		\
+		struct hist_entry *pair;					\
+		int nr_members = evsel->nr_members;				\
+		double *percents;						\
+										\
+		if (nr_members <= 1)						\
+			return ret;						\
+										\
+		percents = zalloc(sizeof(*percents) * nr_members);		\
+		if (percents == NULL) {						\
+			pr_warning("Not enough memory!\n");			\
+			return ret;						\
+		}								\
+										\
+		list_for_each_entry(pair, &he->pairs.head, pairs.node) { 	\
+			u64 period = pair->stat._field;				\
+			u64 total = pair->hists->stats.total_period;		\
+										\
+			if (!total)						\
+				continue;					\
+										\
+			evsel = hists_to_evsel(pair->hists);			\
+			i = perf_evsel__group_idx(evsel);			\
+			percents[i] = 100.0 * period / total;			\
+		}								\
+										\
+		for (i = 1; i < nr_members; i++) {				\
+			ret += percent_color_snprintf(hpp->buf + ret,		\
+						      hpp->size - ret,		\
+						      " %6.2f%%", percents[i]);	\
+		}								\
+		free(percents);							\
+	}									\
+	return ret;								\
+}
+
+#define __HPP_ENTRY_PERCENT_FN(_type, _field)					\
+static int hpp__entry_##_type(struct perf_hpp *hpp, struct hist_entry *he) 	\
+{										\
+	int ret;								\
+	double percent = 0.0;							\
+	struct hists *hists = he->hists;					\
+	const char *fmt = symbol_conf.field_sep ? " %.2f" : " %6.2f%%";		\
+										\
+	if (hists->stats.total_period)						\
+		percent = 100.0 * he->stat._field / hists->stats.total_period; 	\
+										\
+	ret = scnprintf(hpp->buf, hpp->size, fmt, percent);			\
+										\
+	if (symbol_conf.event_group) {						\
+		int i;								\
+		struct perf_evsel *evsel = hists_to_evsel(hists);		\
+		struct hist_entry *pair;					\
+		int nr_members = evsel->nr_members;				\
+		double *percents;						\
+										\
+		if (nr_members <= 1)						\
+			return ret;						\
+										\
+		percents = zalloc(sizeof(*percents) * nr_members);		\
+		if (percents == NULL) {						\
+			pr_warning("Not enough memory!\n");			\
+			return ret;						\
+		}								\
+										\
+		list_for_each_entry(pair, &he->pairs.head, pairs.node) { 	\
+			u64 period = pair->stat._field;				\
+			u64 total = pair->hists->stats.total_period;		\
+										\
+			if (!total)						\
+				continue;					\
+										\
+			evsel = hists_to_evsel(pair->hists);			\
+			i = perf_evsel__group_idx(evsel);			\
+			percents[i] = 100.0 * period / total;			\
+		}								\
+										\
+		for (i = 1; i < nr_members; i++) {				\
+			ret += scnprintf(hpp->buf + ret, hpp->size - ret,	\
+					 fmt, percents[i]);			\
+		}								\
+		free(percents);							\
+	}									\
+	return ret;								\
+}
+
+#define __HPP_ENTRY_RAW_FN(_type, _field)					\
+static int hpp__entry_##_type(struct perf_hpp *hpp, struct hist_entry *he) 	\
+{										\
+	int ret;								\
+	const char *fmt = symbol_conf.field_sep ? " %"PRIu64 : " %11"PRIu64; 	\
+										\
+	ret = scnprintf(hpp->buf, hpp->size, fmt, he->stat._field);		\
+										\
+	if (symbol_conf.event_group) {						\
+		int i;								\
+		struct perf_evsel *evsel = hists_to_evsel(he->hists);		\
+		struct hist_entry *pair;					\
+		int nr_members = evsel->nr_members;				\
+		u64 *numbers;							\
+										\
+		if (nr_members <= 1)						\
+			return ret;						\
+										\
+		numbers = zalloc(sizeof(*numbers) * nr_members);		\
+		if (numbers == NULL) {						\
+			pr_warning("Not enough memory!\n");			\
+			return ret;						\
+		}								\
+										\
+		list_for_each_entry(pair, &he->pairs.head, pairs.node) { 	\
+			evsel = hists_to_evsel(pair->hists);			\
+			i = perf_evsel__group_idx(evsel);			\
+			numbers[i] = pair->stat._field;				\
+		}								\
+										\
+		for (i = 1; i < nr_members; i++) {				\
+			ret += scnprintf(hpp->buf + ret, hpp->size - ret,	\
+					 fmt, numbers[i]);			\
+		}								\
+		free(numbers);							\
+	}									\
+	return ret;								\
+}
+
+#define HPP_PERCENT_FNS(_type, _str, _field, _min_width, _unit_width)	\
+__HPP_HEADER_FN(_type, _str, _min_width, _unit_width)			\
+__HPP_WIDTH_FN(_type, _min_width, _unit_width)				\
+__HPP_COLOR_PERCENT_FN(_type, _field)					\
+__HPP_ENTRY_PERCENT_FN(_type, _field)
+
+#define HPP_RAW_FNS(_type, _str, _field, _min_width, _unit_width)	\
+__HPP_HEADER_FN(_type, _str, _min_width, _unit_width)			\
+__HPP_WIDTH_FN(_type, _min_width, _unit_width)				\
+__HPP_ENTRY_RAW_FN(_type, _field)
+
+
+HPP_PERCENT_FNS(overhead, "Overhead", period, 8, 8)
+HPP_PERCENT_FNS(overhead_sys, "sys", period_sys, 8, 8)
+HPP_PERCENT_FNS(overhead_us, "usr", period_sys, 8, 8)
+HPP_PERCENT_FNS(overhead_guest_sys, "guest sys", period_sys, 9, 8)
+HPP_PERCENT_FNS(overhead_guest_us, "guest usr", period_sys, 9, 8)
+
+HPP_RAW_FNS(samples, "Samples", nr_events, 12, 12)
+HPP_RAW_FNS(period, "Period", period, 12, 12)
+
+
+/* diff-specific hpp functions */
 static int hpp__header_baseline(struct perf_hpp *hpp)
 {
 	return scnprintf(hpp->buf, hpp->size, "Baseline");
@@ -196,44 +242,6 @@ static int hpp__entry_baseline(struct perf_hpp *hpp, struct hist_entry *he)
 		return scnprintf(hpp->buf, hpp->size, "            ");
 }
 
-static int hpp__header_samples(struct perf_hpp *hpp)
-{
-	const char *fmt = symbol_conf.field_sep ? "%s" : "%11s";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, "Samples");
-}
-
-static int hpp__width_samples(struct perf_hpp *hpp __maybe_unused)
-{
-	return 11;
-}
-
-static int hpp__entry_samples(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	const char *fmt = symbol_conf.field_sep ? "%" PRIu64 : "%11" PRIu64;
-
-	return scnprintf(hpp->buf, hpp->size, fmt, he->stat.nr_events);
-}
-
-static int hpp__header_period(struct perf_hpp *hpp)
-{
-	const char *fmt = symbol_conf.field_sep ? "%s" : "%12s";
-
-	return scnprintf(hpp->buf, hpp->size, fmt, "Period");
-}
-
-static int hpp__width_period(struct perf_hpp *hpp __maybe_unused)
-{
-	return 12;
-}
-
-static int hpp__entry_period(struct perf_hpp *hpp, struct hist_entry *he)
-{
-	const char *fmt = symbol_conf.field_sep ? "%" PRIu64 : "%12" PRIu64;
-
-	return scnprintf(hpp->buf, hpp->size, fmt, he->stat.period);
-}
-
 static int hpp__header_period_baseline(struct perf_hpp *hpp)
 {
 	const char *fmt = symbol_conf.field_sep ? "%s" : "%12s";
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index f0ee204f99bb..07421d6da5a2 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -3,6 +3,7 @@
 #include "../../util/util.h"
 #include "../../util/hist.h"
 #include "../../util/sort.h"
+#include "../../util/evsel.h"
 
 
 static size_t callchain__fprintf_left_margin(FILE *fp, int left_margin)
@@ -346,6 +347,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 	struct perf_hpp dummy_hpp = {
 		.buf	= bf,
 		.size	= sizeof(bf),
+		.ptr	= hists_to_evsel(hists),
 	};
 	bool first = true;
 
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 13/18] perf hist browser: Add support for event group view
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (12 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 12/18] perf ui/hist: Add support for event group view Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 14/18] perf gtk/browser: " Namhyung Kim
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Show group members' overhead also when showing the leader's if event
group is enabled.  Use macro for defining hpp functions which looks
almost identical.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 171 ++++++++++++++++++++++++++++++++++-------
 tools/perf/ui/hist.c           |   5 +-
 2 files changed, 146 insertions(+), 30 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index ccc4bd161420..7ac47adc8e09 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -567,23 +567,145 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
 	return row - first_row;
 }
 
-#define HPP__COLOR_FN(_name, _field)					\
-static int hist_browser__hpp_color_ ## _name(struct perf_hpp *hpp,	\
-					     struct hist_entry *he)	\
-{									\
-	struct hists *hists = he->hists;				\
-	double percent = 100.0 * he->stat._field / hists->stats.total_period; \
-	*(double *)hpp->ptr = percent;					\
-	return scnprintf(hpp->buf, hpp->size, "%6.2f%%", percent);	\
+struct hpp_arg {
+	struct ui_browser *b;
+	char folded_sign;
+	bool current_entry;
+};
+
+static int hist_browser__hpp_color_overhead(struct perf_hpp *hpp,
+					    struct hist_entry *he)
+{
+	int ret;
+	double percent = 0.0;
+	struct hists *hists = he->hists;
+	struct hpp_arg *arg = hpp->ptr;
+
+	if (hists->stats.total_period)
+		percent = 100.0 * he->stat.period / hists->stats.total_period;
+
+	ui_browser__set_percent_color(arg->b, percent, arg->current_entry);
+
+	if (symbol_conf.use_callchain) {
+		slsmg_printf("%c ", arg->folded_sign);
+		ret += 2;
+	}
+
+	ret = scnprintf(hpp->buf, hpp->size, "%6.2f%%", percent);
+	slsmg_printf("%s", hpp->buf);
+
+	if (symbol_conf.event_group) {
+		int i;
+		struct perf_evsel *evsel = hists_to_evsel(hists);
+		struct hist_entry *pair;
+		int nr_members = evsel->nr_members;
+		double *percents;
+
+		if (nr_members <= 1)
+			goto out;
+
+		percents = zalloc(sizeof(*percents) * nr_members);
+		if (percents == NULL) {
+			pr_warning("Not enough memory!\n");
+			goto out;
+		}
+
+		list_for_each_entry(pair, &he->pairs.head, pairs.node) {
+			int idx;
+			u64 period = pair->stat.period;
+			u64 total = pair->hists->stats.total_period;
+
+			if (!total)
+				continue;
+
+			evsel = hists_to_evsel(pair->hists);
+			idx = perf_evsel__group_idx(evsel);
+			percents[idx] = 100.0 * period / total;
+		}
+
+		for (i = 1; i < nr_members; i++) {
+			ui_browser__set_percent_color(arg->b, percents[i],
+						      arg->current_entry);
+			ret += scnprintf(hpp->buf, hpp->size,
+					 " %6.2f%%", percents[i]);
+			slsmg_printf("%s", hpp->buf);
+		}
+		free(percents);
+	}
+out:
+	if (!arg->current_entry || !arg->b->navkeypressed)
+		ui_browser__set_color(arg->b, HE_COLORSET_NORMAL);
+
+	return ret;
+}
+
+#define __HPP_COLOR_PERCENT_FN(_type, _field)					\
+static int hist_browser__hpp_color_##_type(struct perf_hpp *hpp, 		\
+					   struct hist_entry *he)		\
+{										\
+	int ret;								\
+	double percent = 0.0;							\
+	struct hists *hists = he->hists;					\
+	struct hpp_arg *arg = hpp->ptr;						\
+										\
+	if (hists->stats.total_period)						\
+		percent = 100.0 * he->stat._field / hists->stats.total_period;	\
+										\
+	ui_browser__set_percent_color(arg->b, percent, arg->current_entry); 	\
+	ret = scnprintf(hpp->buf, hpp->size, "%6.2f%%", percent);		\
+	slsmg_printf("%s", hpp->buf);						\
+										\
+	if (symbol_conf.event_group) {						\
+		int i;								\
+		struct perf_evsel *evsel = hists_to_evsel(hists);		\
+		struct hist_entry *pair;					\
+		int nr_members = evsel->nr_members;				\
+		double *percents;						\
+										\
+		if (nr_members <= 1)						\
+			goto out;						\
+										\
+		percents = zalloc(sizeof(*percents) * nr_members);		\
+		if (percents == NULL) {						\
+			pr_warning("Not enough memory!\n");			\
+			goto out;						\
+		}								\
+										\
+		list_for_each_entry(pair, &he->pairs.head, pairs.node) { 	\
+			int idx;						\
+			u64 period = pair->stat._field;				\
+			u64 total = pair->hists->stats.total_period;		\
+										\
+			if (!total)						\
+				continue;					\
+										\
+			evsel = hists_to_evsel(pair->hists);			\
+			idx = perf_evsel__group_idx(evsel);			\
+			percents[idx] = 100.0 * period / total;			\
+		}								\
+										\
+		for (i = 1; i < nr_members; i++) {				\
+			ui_browser__set_percent_color(arg->b, percents[i],	\
+						      arg->current_entry); 	\
+			ret += scnprintf(hpp->buf, hpp->size,			\
+					 " %6.2f%%", percents[i]);		\
+			slsmg_printf("%s", hpp->buf);				\
+		}								\
+		free(percents);							\
+	}									\
+out:										\
+	if (!arg->current_entry || !arg->b->navkeypressed)			\
+		ui_browser__set_color(arg->b, HE_COLORSET_NORMAL);		\
+										\
+	return ret;								\
 }
 
-HPP__COLOR_FN(overhead, period)
-HPP__COLOR_FN(overhead_sys, period_sys)
-HPP__COLOR_FN(overhead_us, period_us)
-HPP__COLOR_FN(overhead_guest_sys, period_guest_sys)
-HPP__COLOR_FN(overhead_guest_us, period_guest_us)
+__HPP_COLOR_PERCENT_FN(overhead_sys, period_sys)
+__HPP_COLOR_PERCENT_FN(overhead_us, period_us)
+__HPP_COLOR_PERCENT_FN(overhead_guest_sys, period_guest_sys)
+__HPP_COLOR_PERCENT_FN(overhead_guest_us, period_guest_us)
 
-#undef HPP__COLOR_FN
+#undef __HPP_COLOR_PERCENT_FN
 
 void hist_browser__init_hpp(void)
 {
@@ -606,7 +728,6 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 				    unsigned short row)
 {
 	char s[256];
-	double percent;
 	int i, printed = 0;
 	int width = browser->b.width;
 	char folded_sign = ' ';
@@ -625,9 +746,15 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 	}
 
 	if (row_offset == 0) {
+		struct hpp_arg arg = {
+			.b		= &browser->b,
+			.folded_sign	= folded_sign,
+			.current_entry	= current_entry,
+		};
 		struct perf_hpp hpp = {
 			.buf		= s,
 			.size		= sizeof(s),
+			.ptr		= &arg,
 		};
 
 		ui_browser__gotorc(&browser->b, row, 0);
@@ -643,21 +770,7 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 			first = false;
 
 			if (perf_hpp__format[i].color) {
-				hpp.ptr = &percent;
-				/* It will set percent for us. See HPP__COLOR_FN above. */
 				width -= perf_hpp__format[i].color(&hpp, entry);
-
-				ui_browser__set_percent_color(&browser->b, percent, current_entry);
-
-				if (i == PERF_HPP__OVERHEAD && symbol_conf.use_callchain) {
-					slsmg_printf("%c ", folded_sign);
-					width -= 2;
-				}
-
-				slsmg_printf("%s", s);
-
-				if (!current_entry || !browser->b.navkeypressed)
-					ui_browser__set_color(&browser->b, HE_COLORSET_NORMAL);
 			} else {
 				width -= perf_hpp__format[i].entry(&hpp, entry);
 				slsmg_printf("%s", s);
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index b42bd15af3f5..118274f29bb7 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -514,6 +514,9 @@ unsigned int hists__sort_list_width(struct hists *hists)
 {
 	struct sort_entry *se;
 	int i, ret = 0;
+	struct perf_hpp dummy_hpp = {
+		.ptr	= hists_to_evsel(hists),
+	};
 
 	for (i = 0; i < PERF_HPP__MAX_INDEX; i++) {
 		if (!perf_hpp__format[i].cond)
@@ -521,7 +524,7 @@ unsigned int hists__sort_list_width(struct hists *hists)
 		if (i)
 			ret += 2;
 
-		ret += perf_hpp__format[i].width(NULL);
+		ret += perf_hpp__format[i].width(&dummy_hpp);
 	}
 
 	list_for_each_entry(se, &hist_entry__sort_list, list)
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 14/18] perf gtk/browser: Add support for event group view
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (13 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 13/18] perf hist browser: " Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 15/18] perf report: Bypass non-leader events when event group is enabled Namhyung Kim
                   ` (4 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim, Pekka Enberg

From: Namhyung Kim <namhyung.kim@lge.com>

Show group members' overhead also when showing the leader's if event
group is enabled.  Use macro for defining hpp functions which looks
almost identical.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Pekka Enberg <penberg@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/gtk/browser.c | 87 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 17 deletions(-)

diff --git a/tools/perf/ui/gtk/browser.c b/tools/perf/ui/gtk/browser.c
index 253b6219a39e..04ed0cbbc090 100644
--- a/tools/perf/ui/gtk/browser.c
+++ b/tools/perf/ui/gtk/browser.c
@@ -45,30 +45,82 @@ static const char *perf_gtk__get_percent_color(double percent)
 	return NULL;
 }
 
-#define HPP__COLOR_FN(_name, _field)						\
-static int perf_gtk__hpp_color_ ## _name(struct perf_hpp *hpp,			\
-					 struct hist_entry *he)			\
+static int perf_gtk__percent_color_snprintf(char *buf, size_t size,
+					    double percent)
+{
+	int ret = 0;
+	const char *markup;
+
+	markup = perf_gtk__get_percent_color(percent);
+	if (markup)
+		ret += scnprintf(buf, size, markup);
+
+	ret += scnprintf(buf + ret, size - ret, "%6.2f%%", percent);
+
+	if (markup)
+		ret += scnprintf(buf + ret, size - ret, "</span>");
+
+	return ret;
+}
+
+
+#define __HPP_COLOR_PERCENT_FN(_type, _field)					\
+static int perf_gtk__hpp_color_##_type(struct perf_hpp *hpp,			\
+				       struct hist_entry *he)			\
 {										\
+	int ret;								\
+	double percent = 0.0;							\
 	struct hists *hists = he->hists;					\
-	double percent = 100.0 * he->stat._field / hists->stats.total_period;	\
-	const char *markup;							\
-	int ret = 0;								\
 										\
-	markup = perf_gtk__get_percent_color(percent);				\
-	if (markup)								\
-		ret += scnprintf(hpp->buf, hpp->size, "%s", markup);		\
-	ret += scnprintf(hpp->buf + ret, hpp->size - ret, "%6.2f%%", percent); 	\
-	if (markup)								\
-		ret += scnprintf(hpp->buf + ret, hpp->size - ret, "</span>"); 	\
+	if (hists->stats.total_period)						\
+		percent = 100.0 * he->stat._field / hists->stats.total_period; 	\
+										\
+	ret = perf_gtk__percent_color_snprintf(hpp->buf, hpp->size, percent);	\
+										\
+	if (symbol_conf.event_group) {						\
+		int i;								\
+		struct perf_evsel *evsel = hists_to_evsel(hists);		\
+		struct hist_entry *pair;					\
+		int nr_members = evsel->nr_members;				\
+		double *percents;						\
+										\
+		if (nr_members <= 1)						\
+			return ret;						\
+										\
+		percents = zalloc(sizeof(*percents) * nr_members);		\
+		if (percents == NULL) {						\
+			pr_warning("Not enough memory!\n");			\
+			return ret;						\
+		}								\
+										\
+		list_for_each_entry(pair, &he->pairs.head, pairs.node) { 	\
+			u64 period = pair->stat._field;				\
+			u64 total = pair->hists->stats.total_period;		\
+										\
+			if (!total)						\
+				continue;					\
+										\
+			evsel = hists_to_evsel(pair->hists);			\
+			i = perf_evsel__group_idx(evsel);			\
+			percents[i] = 100.0 * period / total;			\
+		}								\
 										\
+		for (i = 1; i < nr_members; i++) {				\
+			ret += scnprintf(hpp->buf + ret, hpp->size -ret, " ");	\
+			ret += perf_gtk__percent_color_snprintf(hpp->buf + ret,	\
+								hpp->size - ret,\
+								percents[i]);	\
+		}								\
+		free(percents);							\
+	}									\
 	return ret;								\
 }
 
-HPP__COLOR_FN(overhead, period)
-HPP__COLOR_FN(overhead_sys, period_sys)
-HPP__COLOR_FN(overhead_us, period_us)
-HPP__COLOR_FN(overhead_guest_sys, period_guest_sys)
-HPP__COLOR_FN(overhead_guest_us, period_guest_us)
+__HPP_COLOR_PERCENT_FN(overhead, period)
+__HPP_COLOR_PERCENT_FN(overhead_sys, period_sys)
+__HPP_COLOR_PERCENT_FN(overhead_us, period_us)
+__HPP_COLOR_PERCENT_FN(overhead_guest_sys, period_guest_sys)
+__HPP_COLOR_PERCENT_FN(overhead_guest_us, period_guest_us)
 
 #undef HPP__COLOR_FN
 
@@ -103,6 +155,7 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists)
 	struct perf_hpp hpp = {
 		.buf		= s,
 		.size		= sizeof(s),
+		.ptr		= hists_to_evsel(hists),
 	};
 
 	nr_cols = 0;
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 15/18] perf report: Bypass non-leader events when event group is enabled
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (14 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 14/18] perf gtk/browser: " Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 16/18] perf report: Show group description " Namhyung Kim
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim, Pekka Enberg

From: Namhyung Kim <namhyung.kim@lge.com>

Since we have all necessary information in the leader events and
other members don't, bypass members.  Member events will be shown
along with the leaders if event group is enabled.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Pekka Enberg <penberg@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c    |  4 ++++
 tools/perf/ui/browsers/hists.c | 41 +++++++++++++++++++++++++++++++++++------
 tools/perf/ui/gtk/browser.c    |  4 ++++
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 13a2cd20a1cd..87af38bd75a8 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -319,6 +319,10 @@ static int perf_evlist__tty_browse_hists(struct perf_evlist *evlist,
 		struct hists *hists = &pos->hists;
 		const char *evname = perf_evsel__name(pos);
 
+		if (symbol_conf.event_group &&
+		    !perf_evsel__is_group_leader(pos))
+			continue;
+
 		hists__fprintf_nr_sample_events(hists, evname, stdout);
 		hists__fprintf(hists, true, 0, 0, stdout);
 		fprintf(stdout, "\n\n");
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7ac47adc8e09..0d8b0143cd9b 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1717,8 +1717,19 @@ out:
 	return key;
 }
 
+static bool filter_group_entries(struct ui_browser *self __maybe_unused,
+				 void *entry)
+{
+	struct perf_evsel *evsel = list_entry(entry, struct perf_evsel, node);
+
+	if (symbol_conf.event_group && !perf_evsel__is_group_leader(evsel))
+		return true;
+
+	return false;
+}
+
 static int __perf_evlist__tui_browse_hists(struct perf_evlist *evlist,
-					   const char *help,
+					   int nr_entries, const char *help,
 					   struct hist_browser_timer *hbt,
 					   struct perf_session_env *env)
 {
@@ -1729,7 +1740,8 @@ static int __perf_evlist__tui_browse_hists(struct perf_evlist *evlist,
 			.refresh    = ui_browser__list_head_refresh,
 			.seek	    = ui_browser__list_head_seek,
 			.write	    = perf_evsel_menu__write,
-			.nr_entries = evlist->nr_entries,
+			.filter	    = filter_group_entries,
+			.nr_entries = nr_entries,
 			.priv	    = evlist,
 		},
 		.env = env,
@@ -1745,20 +1757,37 @@ static int __perf_evlist__tui_browse_hists(struct perf_evlist *evlist,
 			menu.b.width = line_len;
 	}
 
-	return perf_evsel_menu__run(&menu, evlist->nr_entries, help, hbt);
+	return perf_evsel_menu__run(&menu, nr_entries, help, hbt);
 }
 
 int perf_evlist__tui_browse_hists(struct perf_evlist *evlist, const char *help,
 				  struct hist_browser_timer *hbt,
 				  struct perf_session_env *env)
 {
-	if (evlist->nr_entries == 1) {
+	int nr_entries = evlist->nr_entries;
+
+single_entry:
+	if (nr_entries == 1) {
 		struct perf_evsel *first = list_entry(evlist->entries.next,
 						      struct perf_evsel, node);
 		const char *ev_name = perf_evsel__name(first);
-		return perf_evsel__hists_browse(first, evlist->nr_entries, help,
+
+		return perf_evsel__hists_browse(first, nr_entries, help,
 						ev_name, false, hbt, env);
 	}
 
-	return __perf_evlist__tui_browse_hists(evlist, help, hbt, env);
+	if (symbol_conf.event_group) {
+		struct perf_evsel *pos;
+
+		nr_entries = 0;
+		list_for_each_entry(pos, &evlist->entries, node)
+			if (perf_evsel__is_group_leader(pos))
+				nr_entries++;
+
+		if (nr_entries == 1)
+			goto single_entry;
+	}
+
+	return __perf_evlist__tui_browse_hists(evlist, nr_entries, help,
+					       hbt, env);
 }
diff --git a/tools/perf/ui/gtk/browser.c b/tools/perf/ui/gtk/browser.c
index 04ed0cbbc090..3b1642dc0e31 100644
--- a/tools/perf/ui/gtk/browser.c
+++ b/tools/perf/ui/gtk/browser.c
@@ -325,6 +325,10 @@ int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist,
 		GtkWidget *scrolled_window;
 		GtkWidget *tab_label;
 
+		if (symbol_conf.event_group &&
+		    !perf_evsel__is_group_leader(pos))
+			continue;
+
 		scrolled_window = gtk_scrolled_window_new(NULL, NULL);
 
 		gtk_scrolled_window_set_policy(GTK_SCROLLED_WINDOW(scrolled_window),
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 16/18] perf report: Show group description when event group is enabled
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (15 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 15/18] perf report: Bypass non-leader events when event group is enabled Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 17/18] perf report: Add --group option Namhyung Kim
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim, Pekka Enberg

From: Namhyung Kim <namhyung.kim@lge.com>

When using event group viewer, it's better to show the group
description rather than the leader information alone.

If a leader did not contain any member, it's a non-group event.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Pekka Enberg <penberg@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c    | 15 +++++++++++++++
 tools/perf/ui/browsers/hists.c | 25 +++++++++++++++++++++++++
 tools/perf/ui/gtk/browser.c    | 14 +++++++++++---
 tools/perf/util/evsel.c        | 25 +++++++++++++++++++++++++
 tools/perf/util/evsel.h        |  8 ++++++++
 5 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 87af38bd75a8..e5b53f6554f4 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -299,6 +299,21 @@ static size_t hists__fprintf_nr_sample_events(struct hists *self,
 	char unit;
 	unsigned long nr_samples = self->stats.nr_events[PERF_RECORD_SAMPLE];
 	u64 nr_events = self->stats.total_period;
+	struct perf_evsel *evsel = hists_to_evsel(self);
+	char buf[512];
+	size_t size = sizeof(buf);
+
+	if (symbol_conf.event_group && evsel->nr_members > 1) {
+		struct perf_evsel *pos;
+
+		perf_evsel__group_desc(evsel, buf, size);
+		evname = buf;
+
+		for_each_group_member(pos, evsel) {
+			nr_samples += pos->hists.stats.nr_events[PERF_RECORD_SAMPLE];
+			nr_events += pos->hists.stats.total_period;
+		}
+	}
 
 	nr_samples = convert_unit(nr_samples, &unit);
 	ret = fprintf(fp, "# Samples: %lu%c", nr_samples, unit);
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 0d8b0143cd9b..3e31534fcc47 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1211,6 +1211,21 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size,
 	const struct thread *thread = hists->thread_filter;
 	unsigned long nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
 	u64 nr_events = hists->stats.total_period;
+	struct perf_evsel *evsel = hists_to_evsel(hists);
+	char buf[512];
+	size_t buflen = sizeof(buf);
+
+	if (symbol_conf.event_group && evsel->nr_members > 1) {
+		struct perf_evsel *pos;
+
+		perf_evsel__group_desc(evsel, buf, buflen);
+		ev_name = buf;
+
+		for_each_group_member(pos, evsel) {
+			nr_samples += pos->hists.stats.nr_events[PERF_RECORD_SAMPLE];
+			nr_events += pos->hists.stats.total_period;
+		}
+	}
 
 	nr_samples = convert_unit(nr_samples, &unit);
 	printed = scnprintf(bf, size,
@@ -1607,6 +1622,16 @@ static void perf_evsel_menu__write(struct ui_browser *browser,
 	ui_browser__set_color(browser, current_entry ? HE_COLORSET_SELECTED :
 						       HE_COLORSET_NORMAL);
 
+	if (symbol_conf.event_group && evsel->nr_members > 1) {
+		struct perf_evsel *pos;
+
+		ev_name = perf_evsel__group_name(evsel);
+
+		for_each_group_member(pos, evsel) {
+			nr_events += pos->hists.stats.nr_events[PERF_RECORD_SAMPLE];
+		}
+	}
+
 	nr_events = convert_unit(nr_events, &unit);
 	printed = scnprintf(bf, sizeof(bf), "%lu%c%s%s", nr_events,
 			   unit, unit == ' ' ? "" : " ", ev_name);
diff --git a/tools/perf/ui/gtk/browser.c b/tools/perf/ui/gtk/browser.c
index 3b1642dc0e31..e7e11df829ed 100644
--- a/tools/perf/ui/gtk/browser.c
+++ b/tools/perf/ui/gtk/browser.c
@@ -324,10 +324,18 @@ int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist,
 		const char *evname = perf_evsel__name(pos);
 		GtkWidget *scrolled_window;
 		GtkWidget *tab_label;
+		char buf[512];
+		size_t size = sizeof(buf);
 
-		if (symbol_conf.event_group &&
-		    !perf_evsel__is_group_leader(pos))
-			continue;
+		if (symbol_conf.event_group) {
+			if (!perf_evsel__is_group_leader(pos))
+				continue;
+
+			if (pos->nr_members > 1) {
+				perf_evsel__group_desc(pos, buf, size);
+				evname = buf;
+			}
+		}
 
 		scrolled_window = gtk_scrolled_window_new(NULL, NULL);
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 73e6085a7294..a35e2547a7eb 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -405,6 +405,31 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
 	return evsel->name ?: "unknown";
 }
 
+const char *perf_evsel__group_name(struct perf_evsel *evsel)
+{
+	return evsel->group_name ?: "anon group";
+}
+
+int perf_evsel__group_desc(struct perf_evsel *evsel, char *buf, size_t size)
+{
+	int ret;
+	struct perf_evsel *pos;
+	const char *group_name = perf_evsel__group_name(evsel);
+
+	ret = scnprintf(buf, size, "%s", group_name);
+
+	ret += scnprintf(buf + ret, size - ret, " { %s",
+			 perf_evsel__name(evsel));
+
+	for_each_group_member(pos, evsel)
+		ret += scnprintf(buf + ret, size - ret, ", %s",
+				 perf_evsel__name(pos));
+
+	ret += scnprintf(buf + ret, size - ret, " }");
+
+	return ret;
+}
+
 /*
  * The enable_on_exec/disabled value strategy:
  *
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 54a8efbd8dcb..d2d1870928df 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -113,6 +113,8 @@ extern const char *perf_evsel__sw_names[PERF_COUNT_SW_MAX];
 int __perf_evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result,
 					    char *bf, size_t size);
 const char *perf_evsel__name(struct perf_evsel *evsel);
+const char *perf_evsel__group_name(struct perf_evsel *evsel);
+int perf_evsel__group_desc(struct perf_evsel *evsel, char *buf, size_t size);
 
 int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads);
 int perf_evsel__alloc_id(struct perf_evsel *evsel, int ncpus, int nthreads);
@@ -238,4 +240,10 @@ static inline int perf_evsel__group_idx(struct perf_evsel *evsel)
 {
 	return evsel->idx - evsel->leader->idx;
 }
+
+#define for_each_group_member(_evsel, _leader) 					\
+for ((_evsel) = list_entry((_leader)->node.next, struct perf_evsel, node); 	\
+     (_evsel) && (_evsel)->leader == (_leader);					\
+     (_evsel) = list_entry((_evsel)->node.next, struct perf_evsel, node))
+
 #endif /* __PERF_EVSEL_H */
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 17/18] perf report: Add --group option
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (16 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 16/18] perf report: Show group description " Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:38 ` [PATCH 18/18] perf report: Add report.group config option Namhyung Kim
  2012-11-29  6:44 ` [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Add --group option to enable event grouping.  When enabled, all the
group members information will be shown together with the leader.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-report.txt | 3 +++
 tools/perf/builtin-report.c              | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index f4d91bebd59d..b3e8fcb9bbce 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -171,6 +171,9 @@ OPTIONS
 --objdump=<path>::
         Path to objdump binary.
 
+--group::
+	Show event group information together.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-annotate[1]
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index e5b53f6554f4..c7079dff7411 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -668,6 +668,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
 	OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
 		    "Show a column with the sum of periods"),
+	OPT_BOOLEAN(0, "group", &symbol_conf.event_group,
+		    "Show event group information together"),
 	OPT_CALLBACK_NOOPT('b', "branch-stack", &sort__branch_mode, "",
 		    "use branch records for histogram filling", parse_branch_mode),
 	OPT_STRING(0, "objdump", &objdump_path, "path",
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 18/18] perf report: Add report.group config option
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (17 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 17/18] perf report: Add --group option Namhyung Kim
@ 2012-11-29  6:38 ` Namhyung Kim
  2012-11-29  6:44 ` [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

From: Namhyung Kim <namhyung.kim@lge.com>

Add report.group config option for setting default value of event
group view.  It affects the report output only if perf.data contains
event group info.

A user can write .perfconfig file like below to enable group view by
default:

  $ cat ~/.perfconfig
  [report]
  group = true

And it can be disabled through command line:

  $ perf report --no-group

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index c7079dff7411..f35e489156dc 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -8,6 +8,7 @@
 #include "builtin.h"
 
 #include "util/util.h"
+#include "util/cache.h"
 
 #include "util/annotate.h"
 #include "util/color.h"
@@ -54,6 +55,16 @@ struct perf_report {
 	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
 };
 
+static int perf_report_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "report.group")) {
+		symbol_conf.event_group = perf_config_bool(var, value);
+		return 0;
+	}
+
+	return perf_default_config(var, value, cb);
+}
+
 static int perf_report__add_branch_hist_entry(struct perf_tool *tool,
 					struct addr_location *al,
 					struct perf_sample *sample,
@@ -677,6 +688,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_END()
 	};
 
+	perf_config(perf_report_config, NULL);
+
 	argc = parse_options(argc, argv, options, report_usage, 0);
 
 	if (report.use_stdio)
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH 00/18] perf report: Add support for event group view (v6)
  2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
                   ` (18 preceding siblings ...)
  2012-11-29  6:38 ` [PATCH 18/18] perf report: Add report.group config option Namhyung Kim
@ 2012-11-29  6:44 ` Namhyung Kim
  19 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29  6:44 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen

Hi all,

This is my 6th iteration of event group view patchset.
For basic idea and usage example, please see my original post [1].

The largest change in this version is using 'pairs' list of hist_entry
instead of allocating group_stats for all group members.  But I needed
to allocate temporary arrays for internal purpose anyway.  If you have
a better solution please let me know.

Jiri, sorry for the delay.  I had to hunt down some nasty bugs in my
patches.  And I saw you submitted the multiplu diff series which will
conflict my changes.  I'll have a look at it soon, and I'd be glad if
you take a look at my changes too. :)

You can get this series via my tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git  perf/group-v6

Any comments are welcome, thanks,
Namhyung


v5 -> v6:
 * set ->leader alse for leader evsel (Arnaldo)
 * use hists__{match,link} (Arnaldo)

v4 -> v5:
 * rebase onto acme/perf/core

v3 -> v4:
 * patch 1-9 in previous post are merged.
 * add Jiri's Acked-by
 * add report.group config option

v2 -> v3:
 * drop patch 1 since it's merged into acme/perf/core
 * cherry-pick Jiri's hpp changes
 * add missing bswap_32 on reading nr_groups (Jiri)
 * remove perf_evlist__recalc_nr_groups() in favor of list_is_last (Jiri)

v1 -> v2:
 * save group relation to header (Jiri)

[1] https://lkml.org/lkml/2012/7/24/81


Namhyung Kim (18):
  perf evsel: Set leader evsel's ->leader to itself
  perf evsel: Convert to _is_group_leader method
  perf tools: Keep group information
  perf header: Add HEADER_GROUP_DESC feature
  perf tools: Fix typo on hist__entry_add_pair
  perf hists: Link hist entry pairs to leader
  perf hists: Exchange order of comparing items when collapsing hists
  perf hists: Add hists__{match,link}_collapsed
  perf symbol: Introduce symbol_conf.event_group
  perf report: Make another loop for linking group hists
  perf hists: Resort hist entries using group members for output
  perf ui/hist: Add support for event group view
  perf hist browser: Add support for event group view
  perf gtk/browser: Add support for event group view
  perf report: Bypass non-leader events when event group is enabled
  perf report: Show group description when event group is enabled
  perf report: Add --group option
  perf report: Add report.group config option

 tools/perf/Documentation/perf-report.txt |   3 +
 tools/perf/builtin-record.c              |   3 +
 tools/perf/builtin-report.c              |  47 +++-
 tools/perf/builtin-stat.c                |   2 +-
 tools/perf/tests/parse-events.c          |  20 +-
 tools/perf/ui/browsers/hists.c           | 237 ++++++++++++++++---
 tools/perf/ui/gtk/browser.c              |  99 ++++++--
 tools/perf/ui/hist.c                     | 375 ++++++++++++++++---------------
 tools/perf/ui/stdio/hist.c               |   2 +
 tools/perf/util/evlist.c                 |  12 +-
 tools/perf/util/evlist.h                 |   1 +
 tools/perf/util/evsel.c                  |  32 ++-
 tools/perf/util/evsel.h                  |  20 +-
 tools/perf/util/header.c                 | 153 +++++++++++++
 tools/perf/util/header.h                 |   2 +
 tools/perf/util/hist.c                   | 182 +++++++++++++--
 tools/perf/util/hist.h                   |   2 +
 tools/perf/util/parse-events.c           |   1 +
 tools/perf/util/parse-events.h           |   1 +
 tools/perf/util/parse-events.y           |  10 +
 tools/perf/util/sort.h                   |   2 +-
 tools/perf/util/symbol.c                 |   4 +
 tools/perf/util/symbol.h                 |   3 +-
 23 files changed, 934 insertions(+), 279 deletions(-)

-- 
1.7.11.7


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 00/18] perf report: Add support for event group view (v6)
  2012-11-29  6:38 ` Namhyung Kim
@ 2012-11-29 12:21   ` Jiri Olsa
  0 siblings, 0 replies; 50+ messages in thread
From: Jiri Olsa @ 2012-11-29 12:21 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen

On Thu, Nov 29, 2012 at 03:38:28PM +0900, Namhyung Kim wrote:
> Hi all,
> 
> This is my 6th iteration of event group view patchset.
> For basic idea and usage example, please see my original post [1].
> 
> The largest change in this version is using 'pairs' list of hist_entry
> instead of allocating group_stats for all group members.  But I needed
> to allocate temporary arrays for internal purpose anyway.  If you have
> a better solution please let me know.
> 
> Jiri, sorry for the delay, I had to hunt down some nasty bugs in my
> patches.  And I saw you submitted the multiplu diff series which will
> conflict my changes.  I'll have a look at it soon, and I'd be glad if
> you take a look at my changes too. :)

np, great.. will do ;)

thanks,
jirka

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 01/18] perf evsel: Set leader evsel's ->leader to itself
  2012-11-29  6:38 ` [PATCH 01/18] perf evsel: Set leader evsel's ->leader to itself Namhyung Kim
@ 2012-11-29 14:28   ` Jiri Olsa
  2013-01-24 18:48   ` [tip:perf/core] " tip-bot for Namhyung Kim
  1 sibling, 0 replies; 50+ messages in thread
From: Jiri Olsa @ 2012-11-29 14:28 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Thu, Nov 29, 2012 at 03:38:29PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Currently only non-leader members are set ->leader to the leader evsel
> of the group and the leader has set NULL.  Thus it requires special
> casing for leader evsels.  Set ->leader to itself will remove this.
> 
> Suggested-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Stephane Eranian <eranian@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---

Acked-by: Jiri Olsa <jolsa@redhat.com>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 02/18] perf evsel: Convert to _is_group_leader method
  2012-11-29  6:38 ` [PATCH 02/18] perf evsel: Convert to _is_group_leader method Namhyung Kim
@ 2012-11-29 14:28   ` Jiri Olsa
  2013-01-24 18:49   ` [tip:perf/core] " tip-bot for Namhyung Kim
  1 sibling, 0 replies; 50+ messages in thread
From: Jiri Olsa @ 2012-11-29 14:28 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Thu, Nov 29, 2012 at 03:38:30PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Convert perf_evsel__is_group_member to perf_evsel__is_group_leader.
> This is because the most usecases are using negative form to check
> whether the given evsel is a leader or not and it's IMHO somewhat
> ambiguous - leader also *is* a member of the group.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---

Acked-by: Jiri Olsa <jolsa@redhat.com>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 03/18] perf tools: Keep group information
  2012-11-29  6:38 ` [PATCH 03/18] perf tools: Keep group information Namhyung Kim
@ 2012-11-29 14:33   ` Jiri Olsa
  2012-11-29 14:58     ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Jiri Olsa @ 2012-11-29 14:33 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Thu, Nov 29, 2012 at 03:38:31PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>

SNIP

> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -21,6 +21,7 @@ struct perf_evlist {
>  	struct list_head entries;
>  	struct hlist_head heads[PERF_EVLIST__HLIST_SIZE];
>  	int		 nr_entries;
> +	int		 nr_groups;
>  	int		 nr_fds;
>  	int		 nr_mmaps;
>  	int		 mmap_len;
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 46c8004ca56b..887834ed0af1 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -73,6 +73,7 @@ struct perf_evsel {
>  	bool 			needs_swap;
>  	/* parse modifier helper */
>  	int			exclude_GH;
> +	int			nr_members;
>  	struct perf_evsel	*leader;
>  	char			*group_name;
>  };

we could test both new fields in existing group tests

> @@ -230,4 +231,9 @@ static inline bool perf_evsel__is_group_leader(const struct perf_evsel *evsel)
>  {
>  	return evsel->leader == evsel;

SNIP

>  {
>  	struct list_head *list = $3;
> +	struct parse_events_data__events *data = _data;
> +
> +	/* Count groups only have more than 1 members */
> +	if (!list_is_last(list->next, list))
> +		data->nr_groups++;
>  
>  	parse_events__set_leader($1, list);
>  	$$ = list;
> @@ -130,6 +135,11 @@ PE_NAME '{' events '}'
>  '{' events '}'
>  {
>  	struct list_head *list = $2;
> +	struct parse_events_data__events *data = _data;
> +
> +	/* Count groups only have more than 1 members */
> +	if (!list_is_last(list->next, list))
> +		data->nr_groups++;

nitpick.. maybe add a function for above 3 lines?

You could add static function right after ABORT_ON
macro definition.

thanks,
jirka

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 05/18] perf tools: Fix typo on hist__entry_add_pair
  2012-11-29  6:38 ` [PATCH 05/18] perf tools: Fix typo on hist__entry_add_pair Namhyung Kim
@ 2012-11-29 14:33   ` Jiri Olsa
  0 siblings, 0 replies; 50+ messages in thread
From: Jiri Olsa @ 2012-11-29 14:33 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Thu, Nov 29, 2012 at 03:38:33PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Fix a misplaced underscore.  In this case, 'hist_entry' is the name of
> data structure and we usually put double underscores between data
> structure and actual function name.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---

Acked-by: Jiri Olsa <jolsa@redhat.com>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 03/18] perf tools: Keep group information
  2012-11-29 14:33   ` Jiri Olsa
@ 2012-11-29 14:58     ` Namhyung Kim
  2012-11-29 15:02       ` Jiri Olsa
  0 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29 14:58 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

2012-11-29 (목), 15:33 +0100, Jiri Olsa:
> On Thu, Nov 29, 2012 at 03:38:31PM +0900, Namhyung Kim wrote:
> > From: Namhyung Kim <namhyung.kim@lge.com>
> 
> SNIP
> 
> > --- a/tools/perf/util/evlist.h
> > +++ b/tools/perf/util/evlist.h
> > @@ -21,6 +21,7 @@ struct perf_evlist {
> >  	struct list_head entries;
> >  	struct hlist_head heads[PERF_EVLIST__HLIST_SIZE];
> >  	int		 nr_entries;
> > +	int		 nr_groups;
> >  	int		 nr_fds;
> >  	int		 nr_mmaps;
> >  	int		 mmap_len;
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 46c8004ca56b..887834ed0af1 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -73,6 +73,7 @@ struct perf_evsel {
> >  	bool 			needs_swap;
> >  	/* parse modifier helper */
> >  	int			exclude_GH;
> > +	int			nr_members;
> >  	struct perf_evsel	*leader;
> >  	char			*group_name;
> >  };
> 
> we could test both new fields in existing group tests
> 
> > @@ -230,4 +231,9 @@ static inline bool perf_evsel__is_group_leader(const struct perf_evsel *evsel)
> >  {
> >  	return evsel->leader == evsel;
> 

You mean adding 'evsel->nr_members > 1' to this function?  For some
reason, I'd like to treat non-group events as group leaders so I dropped
that check from the function.


> SNIP
> 
> >  {
> >  	struct list_head *list = $3;
> > +	struct parse_events_data__events *data = _data;
> > +
> > +	/* Count groups only have more than 1 members */
> > +	if (!list_is_last(list->next, list))
> > +		data->nr_groups++;
> >  
> >  	parse_events__set_leader($1, list);
> >  	$$ = list;
> > @@ -130,6 +135,11 @@ PE_NAME '{' events '}'
> >  '{' events '}'
> >  {
> >  	struct list_head *list = $2;
> > +	struct parse_events_data__events *data = _data;
> > +
> > +	/* Count groups only have more than 1 members */
> > +	if (!list_is_last(list->next, list))
> > +		data->nr_groups++;
> 
> nitpick.. maybe add a function for above 3 lines?
> 
> You could add static function right after ABORT_ON
> macro definition.

No problem.  Will do.

Thanks,
Namhyung



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 03/18] perf tools: Keep group information
  2012-11-29 14:58     ` Namhyung Kim
@ 2012-11-29 15:02       ` Jiri Olsa
  2012-11-29 15:09         ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Jiri Olsa @ 2012-11-29 15:02 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Thu, Nov 29, 2012 at 11:58:54PM +0900, Namhyung Kim wrote:
> 2012-11-29 (목), 15:33 +0100, Jiri Olsa:
> > On Thu, Nov 29, 2012 at 03:38:31PM +0900, Namhyung Kim wrote:
> > > From: Namhyung Kim <namhyung.kim@lge.com>
> > 
> > SNIP
> > 
> > > --- a/tools/perf/util/evlist.h
> > > +++ b/tools/perf/util/evlist.h
> > > @@ -21,6 +21,7 @@ struct perf_evlist {
> > >  	struct list_head entries;
> > >  	struct hlist_head heads[PERF_EVLIST__HLIST_SIZE];
> > >  	int		 nr_entries;
> > > +	int		 nr_groups;
> > >  	int		 nr_fds;
> > >  	int		 nr_mmaps;
> > >  	int		 mmap_len;
> > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > > index 46c8004ca56b..887834ed0af1 100644
> > > --- a/tools/perf/util/evsel.h
> > > +++ b/tools/perf/util/evsel.h
> > > @@ -73,6 +73,7 @@ struct perf_evsel {
> > >  	bool 			needs_swap;
> > >  	/* parse modifier helper */
> > >  	int			exclude_GH;
> > > +	int			nr_members;
> > >  	struct perf_evsel	*leader;
> > >  	char			*group_name;
> > >  };
> > 
> > we could test both new fields in existing group tests
> > 
> > > @@ -230,4 +231,9 @@ static inline bool perf_evsel__is_group_leader(const struct perf_evsel *evsel)
> > >  {
> > >  	return evsel->leader == evsel;
> > 
> 
> You mean adding 'evsel->nr_members > 1' to this function?  For some
> reason, I'd like to treat non-group events as group leaders so I dropped
> that check from the function.

nope, the change is ok, I meant updating automated tests in tests/parse-events.c

thanks,
jirka

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 03/18] perf tools: Keep group information
  2012-11-29 15:02       ` Jiri Olsa
@ 2012-11-29 15:09         ` Namhyung Kim
  2012-11-29 19:09           ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-11-29 15:09 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

2012-11-29 (목), 16:02 +0100, Jiri Olsa:
> On Thu, Nov 29, 2012 at 11:58:54PM +0900, Namhyung Kim wrote:
> > 2012-11-29 (목), 15:33 +0100, Jiri Olsa:
> > > On Thu, Nov 29, 2012 at 03:38:31PM +0900, Namhyung Kim wrote:
> > > > From: Namhyung Kim <namhyung.kim@lge.com>
> > > 
> > > SNIP
> > > 
> > > > --- a/tools/perf/util/evlist.h
> > > > +++ b/tools/perf/util/evlist.h
> > > > @@ -21,6 +21,7 @@ struct perf_evlist {
> > > >  	struct list_head entries;
> > > >  	struct hlist_head heads[PERF_EVLIST__HLIST_SIZE];
> > > >  	int		 nr_entries;
> > > > +	int		 nr_groups;
> > > >  	int		 nr_fds;
> > > >  	int		 nr_mmaps;
> > > >  	int		 mmap_len;
> > > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > > > index 46c8004ca56b..887834ed0af1 100644
> > > > --- a/tools/perf/util/evsel.h
> > > > +++ b/tools/perf/util/evsel.h
> > > > @@ -73,6 +73,7 @@ struct perf_evsel {
> > > >  	bool 			needs_swap;
> > > >  	/* parse modifier helper */
> > > >  	int			exclude_GH;
> > > > +	int			nr_members;
> > > >  	struct perf_evsel	*leader;
> > > >  	char			*group_name;
> > > >  };
> > > 
> > > we could test both new fields in existing group tests
> > > 
> > > > @@ -230,4 +231,9 @@ static inline bool perf_evsel__is_group_leader(const struct perf_evsel *evsel)
> > > >  {
> > > >  	return evsel->leader == evsel;
> > > 
> > 
> > You mean adding 'evsel->nr_members > 1' to this function?  For some
> > reason, I'd like to treat non-group events as group leaders so I dropped
> > that check from the function.
> 
> nope, the change is ok, I meant updating automated tests in tests/parse-events.c

Ah, okay.  Always forgot to update the test. :(  Will add.

Thanks,
Namhyung




^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 04/18] perf header: Add HEADER_GROUP_DESC feature
  2012-11-29  6:38 ` [PATCH 04/18] perf header: Add HEADER_GROUP_DESC feature Namhyung Kim
@ 2012-11-29 18:43   ` Arnaldo Carvalho de Melo
  2012-12-03  1:16     ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Arnaldo Carvalho de Melo @ 2012-11-29 18:43 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

Em Thu, Nov 29, 2012 at 03:38:32PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Save group relationship information so that it can be restored when
> perf report is running.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Stephane Eranian <eranian@google.com>
> Acked-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-record.c |   3 +
>  tools/perf/util/header.c    | 153 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/header.h    |   2 +
>  3 files changed, 158 insertions(+)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f3151d3c70ce..d50f0dcfe1c1 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -592,6 +592,9 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  		goto out_delete_session;
>  	}
>  
> +	if (!evsel_list->nr_groups)
> +		perf_header__clear_feat(&session->header, HEADER_GROUP_DESC);
> +
>  	/*
>  	 * perf_session__delete(session) will be called at perf_record__exit()
>  	 */
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 195a47a8f052..6ae8185842f2 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1073,6 +1073,41 @@ static int write_pmu_mappings(int fd, struct perf_header *h __maybe_unused,
>  }
>  
>  /*
> + * File format:
> + *
> + * struct group_descs {
> + *	u32	nr_groups;
> + *	struct group_desc {
> + *		char	name[];
> + *		u32	leader_idx;
> + *		u32	nr_members;
> + *	}[nr_groups];
> + * };
> + */
> +static int write_group_desc(int fd, struct perf_header *h __maybe_unused,
> +			    struct perf_evlist *evlist)
> +{
> +	u32 nr_groups = evlist->nr_groups;
> +	struct perf_evsel *evsel;
> +
> +	do_write(fd, &nr_groups, sizeof(nr_groups));
> +
> +	list_for_each_entry(evsel, &evlist->entries, node) {
> +		if (perf_evsel__is_group_leader(evsel) &&
> +		    evsel->nr_members > 1) {
> +			const char *name = evsel->group_name ?: "{anon_group}";
> +			u32 leader_idx = evsel->idx;
> +			u32 nr_members = evsel->nr_members;
> +
> +			do_write_string(fd, name);
> +			do_write(fd, &leader_idx, sizeof(leader_idx));
> +			do_write(fd, &nr_members, sizeof(nr_members));

You need to check do_write() return, it can fail. There are some cases
in header.c that don't check it, those should be eventually fixed, most
cases check it tho.

> +		}
> +	}
> +	return 0;
> +}
> +
> +/*
>   * default get_cpuid(): nothing gets recorded
>   * actual implementation must be in arch/$(ARCH)/util/header.c
>   */
> @@ -1433,6 +1468,31 @@ error:
>  	fprintf(fp, "# pmu mappings: unable to read\n");
>  }
>  
> +static void print_group_desc(struct perf_header *ph, int fd __maybe_unused,
> +			     FILE *fp)
> +{
> +	struct perf_session *session;
> +	struct perf_evsel *evsel;
> +	u32 nr = 0;
> +
> +	session = container_of(ph, struct perf_session, header);
> +
> +	list_for_each_entry(evsel, &session->evlist->entries, node) {
> +		if (perf_evsel__is_group_leader(evsel) &&
> +		    evsel->nr_members > 1) {
> +			fprintf(fp, "# group: %s{%s", evsel->group_name ?: "",
> +				perf_evsel__name(evsel));
> +
> +			nr = evsel->nr_members - 1;
> +		} else if (nr) {
> +			fprintf(fp, ",%s", perf_evsel__name(evsel));
> +
> +			if (--nr == 0)
> +				fprintf(fp, "}\n");
> +		}
> +	}
> +}
> +
>  static int __event_process_build_id(struct build_id_event *bev,
>  				    char *filename,
>  				    struct perf_session *session)
> @@ -1947,6 +2007,98 @@ error:
>  	return -1;
>  }
>  
> +static int process_group_desc(struct perf_file_section *section __maybe_unused,
> +			      struct perf_header *ph, int fd,
> +			      void *data __maybe_unused)
> +{
> +	size_t ret = -1;
> +	u32 i, nr, nr_groups;
> +	struct perf_session *session;
> +	struct perf_evsel *evsel, *leader = NULL;
> +	struct group_desc {
> +		char *name;
> +		u32 leader_idx;
> +		u32 nr_members;
> +	} *desc;
> +
> +	if (read(fd, &nr_groups, sizeof(nr_groups)) != sizeof(nr_groups))

Please use readn() instead in all read() cases in this function.

> +		return -1;
> +
> +	if (ph->needs_swap)
> +		nr_groups = bswap_32(nr_groups);
> +
> +	ph->env.nr_groups = nr_groups;
> +	if (!nr_groups) {
> +		pr_debug("group desc not available\n");
> +		return 0;
> +	}
> +
> +	desc = calloc(nr_groups, sizeof(*desc));
> +	if (!desc)
> +		return -1;
> +
> +	for (i = 0; i < nr_groups; i++) {
> +		desc[i].name = do_read_string(fd, ph);
> +		if (!desc[i].name)
> +			goto out_free;
> +
> +		if (read(fd, &desc[i].leader_idx, sizeof(u32)) != sizeof(u32))
> +			goto out_free;
> +
> +		if (read(fd, &desc[i].nr_members, sizeof(u32)) != sizeof(u32))
> +			goto out_free;
> +
> +		if (ph->needs_swap) {
> +			desc[i].leader_idx = bswap_32(desc[i].leader_idx);
> +			desc[i].nr_members = bswap_32(desc[i].nr_members);
> +		}
> +	}
> +
> +	/*
> +	 * Rebuild group relationship based on the group_desc
> +	 */
> +	session = container_of(ph, struct perf_session, header);
> +	session->evlist->nr_groups = nr_groups;
> +
> +	i = nr = 0;
> +	list_for_each_entry(evsel, &session->evlist->entries, node) {
> +		if (evsel->idx == (int) desc[i].leader_idx) {
> +			evsel->leader = evsel;
> +			/* {anon_group} is a dummy name */
> +			if (strcmp(desc[i].name, "{anon_group}"))
> +				evsel->group_name = desc[i].name;
> +			evsel->nr_members = desc[i].nr_members;
> +
> +			if (i >= nr_groups || nr > 0) {
> +				pr_debug("invalid group desc\n");
> +				goto out_free;
> +			}
> +
> +			leader = evsel;
> +			nr = evsel->nr_members - 1;
> +			i++;
> +		} else if (nr) {
> +			/* This is a group member */
> +			evsel->leader = leader;
> +
> +			nr--;
> +		}
> +	}
> +
> +	if (i != nr_groups || nr != 0) {
> +		pr_debug("invalid group desc\n");
> +		goto out_free;
> +	}
> +
> +	ret = 0;
> +out_free:
> +	while ((int) --i >= 0)
> +		free(desc[i].name);
> +	free(desc);
> +
> +	return ret;
> +}
> +
>  struct feature_ops {
>  	int (*write)(int fd, struct perf_header *h, struct perf_evlist *evlist);
>  	void (*print)(struct perf_header *h, int fd, FILE *fp);
> @@ -1986,6 +2138,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
>  	FEAT_OPF(HEADER_NUMA_TOPOLOGY,	numa_topology),
>  	FEAT_OPA(HEADER_BRANCH_STACK,	branch_stack),
>  	FEAT_OPP(HEADER_PMU_MAPPINGS,	pmu_mappings),
> +	FEAT_OPP(HEADER_GROUP_DESC,	group_desc),
>  };
>  
>  struct header_print_data {
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 5f1cd6884f37..e3e7fb490310 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -29,6 +29,7 @@ enum {
>  	HEADER_NUMA_TOPOLOGY,
>  	HEADER_BRANCH_STACK,
>  	HEADER_PMU_MAPPINGS,
> +	HEADER_GROUP_DESC,
>  	HEADER_LAST_FEATURE,
>  	HEADER_FEAT_BITS	= 256,
>  };
> @@ -79,6 +80,7 @@ struct perf_session_env {
>  	char			*numa_nodes;
>  	int			nr_pmu_mappings;
>  	char			*pmu_mappings;
> +	int			nr_groups;
>  };
>  
>  struct perf_header {
> -- 
> 1.7.11.7

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists
  2012-11-29  6:38 ` [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists Namhyung Kim
@ 2012-11-29 18:52   ` Arnaldo Carvalho de Melo
  2012-12-03  1:41     ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Arnaldo Carvalho de Melo @ 2012-11-29 18:52 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

Em Thu, Nov 29, 2012 at 03:38:35PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> When comparing entries for collapsing put the given entry first, and
> then the iterated entry.  This is for the sake of consistency and will

consistency with what? and, see below:

> be required by the event group report.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Stephane Eranian <eranian@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 82df1b26f0d4..161c35e7ed0e 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -433,7 +433,7 @@ static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
>  		parent = *p;
>  		iter = rb_entry(parent, struct hist_entry, rb_node_in);
>  
> -		cmp = hist_entry__collapse(iter, he);
> +		cmp = hist_entry__collapse(he, iter);
>  
>  		if (!cmp) {
>  			he_stat__add_stat(&iter->stat, &he->stat);

doesn't this now gets inconsistent with the hist_entry__collapse() call?
I.e. iter first, he after, also there is the case for callchains, below,
care to elaborate here?

> -- 
> 1.7.11.7

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 03/18] perf tools: Keep group information
  2012-11-29 15:09         ` Namhyung Kim
@ 2012-11-29 19:09           ` Arnaldo Carvalho de Melo
  2012-12-03  1:12             ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Arnaldo Carvalho de Melo @ 2012-11-29 19:09 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Jiri Olsa, Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML,
	Stephane Eranian, Andi Kleen, Namhyung Kim

Em Fri, Nov 30, 2012 at 12:09:11AM +0900, Namhyung Kim escreveu:
> 2012-11-29 (목), 16:02 +0100, Jiri Olsa:
> > > You mean adding 'evsel->nr_members > 1' to this function?  For some
> > > reason, I'd like to treat non-group events as group leaders so I dropped
> > > that check from the function.
> > 
> > nope, the change is ok, I meant updating automated tests in tests/parse-events.c
> 
> Ah, okay.  Always forgot to update the test. :(  Will add.

Also consider adding more tests, perhaps for the formatting, i.e.
calling those hpp functions and then checking that the resulting
formatted string is the one expected.

One thing I need to do is to create a test that uses slang to write a
line with callchains and then, without updating the screen, check if
what is buffered to refresh the screen is what we expect, so that we can
check that that '+' char isn't lost like it happened twice :-)

- Arnaldo

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
       [not found]   ` <20121130132943.GC1080@krava.brq.redhat.com>
@ 2012-11-30 13:52     ` Arnaldo Carvalho de Melo
  2012-12-03  1:51       ` Namhyung Kim
  2012-12-04  9:16       ` Namhyung Kim
  2012-12-03  1:56     ` Namhyung Kim
  1 sibling, 2 replies; 50+ messages in thread
From: Arnaldo Carvalho de Melo @ 2012-11-30 13:52 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Namhyung Kim, Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML,
	Stephane Eranian, Andi Kleen, Namhyung Kim

Em Fri, Nov 30, 2012 at 02:29:43PM +0100, Jiri Olsa escreveu:
> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
> > +#define __HPP_COLOR_PERCENT_FN(_type, _field)					\
> > +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he) 	\
> > +{										\
> > +	int ret;								\
> > +	double percent = 0.0;							\
> > +	struct hists *hists = he->hists;					\
> > +										\
> > +	if (hists->stats.total_period)						\
> > +		percent = 100.0 * he->stat._field / hists->stats.total_period; 	\
> > +										\
> > +	ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent);	\
> > +										\
> > +	if (symbol_conf.event_group) {						\
> > +		int i;								\
> > +		struct perf_evsel *evsel = hists_to_evsel(hists);		\
> > +		struct hist_entry *pair;					\
> > +		int nr_members = evsel->nr_members;				\
> > +		double *percents;						\
> > +										\
> > +		if (nr_members <= 1)						\
> > +			return ret;						\
> > +										\
> > +		percents = zalloc(sizeof(*percents) * nr_members);		\
> > +		if (percents == NULL) {						\
> > +			pr_warning("Not enough memory!\n");			\
> > +			return ret;						\
> > +		}								\

Why do we need to zalloc this percents array?

> > +		list_for_each_entry(pair, &he->pairs.head, pairs.node) { 	\
> > +			u64 period = pair->stat._field;				\
> > +			u64 total = pair->hists->stats.total_period;		\
> > +										\
> > +			if (!total)						\
> > +				continue;					\
> > +										\
> > +			evsel = hists_to_evsel(pair->hists);			\
> > +			i = perf_evsel__group_idx(evsel);			\
> > +			percents[i] = 100.0 * period / total;			\

Why not use percent_color_snprintf here, using some "%*s" thing that
uses i to position the output in the right column? This way the
following loop can be ditched as well. No?

> > +		}								\
> > +										\
> > +		for (i = 1; i < nr_members; i++) {				\
> > +			ret += percent_color_snprintf(hpp->buf + ret,		\
> > +						      hpp->size - ret,		\
> > +						      " %6.2f%%", percents[i]);	\
> > +		}								\
> > +		free(percents);							\
> > +	}									\
> > +	return ret;								\

- Arnaldo

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 03/18] perf tools: Keep group information
  2012-11-29 19:09           ` Arnaldo Carvalho de Melo
@ 2012-12-03  1:12             ` Namhyung Kim
  0 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03  1:12 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML,
	Stephane Eranian, Andi Kleen, Namhyung Kim

Hi Arnaldo,

On Thu, 29 Nov 2012 16:09:22 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 30, 2012 at 12:09:11AM +0900, Namhyung Kim escreveu:
>> 2012-11-29 (목), 16:02 +0100, Jiri Olsa:
>> > > You mean adding 'evsel->nr_members > 1' to this function?  For some
>> > > reason, I'd like to treat non-group events as group leaders so I dropped
>> > > that check from the function.
>> > 
>> > nope, the change is ok, I meant updating automated tests in tests/parse-events.c
>> 
>> Ah, okay.  Always forgot to update the test. :(  Will add.
>
> Also consider adding more tests, perhaps for the formatting, i.e.
> calling those hpp functions and then checking that the resulting
> formatted string is the one expected.
>
> One thing I need to do is to create a test that uses slang to write a
> line with callchains and then, without updating the screen, check if
> what is buffered to refresh the screen is what we expect, so that we can
> check that that '+' char isn't lost like it happened twice :-)

Yep, will try.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 04/18] perf header: Add HEADER_GROUP_DESC feature
  2012-11-29 18:43   ` Arnaldo Carvalho de Melo
@ 2012-12-03  1:16     ` Namhyung Kim
  0 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03  1:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

On Thu, 29 Nov 2012 15:43:04 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 29, 2012 at 03:38:32PM +0900, Namhyung Kim escreveu:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> Save group relationship information so that it can be restored when
>> perf report is running.
>> 
[snip]
>> +static int write_group_desc(int fd, struct perf_header *h __maybe_unused,
>> +			    struct perf_evlist *evlist)
>> +{
>> +	u32 nr_groups = evlist->nr_groups;
>> +	struct perf_evsel *evsel;
>> +
>> +	do_write(fd, &nr_groups, sizeof(nr_groups));
>> +
>> +	list_for_each_entry(evsel, &evlist->entries, node) {
>> +		if (perf_evsel__is_group_leader(evsel) &&
>> +		    evsel->nr_members > 1) {
>> +			const char *name = evsel->group_name ?: "{anon_group}";
>> +			u32 leader_idx = evsel->idx;
>> +			u32 nr_members = evsel->nr_members;
>> +
>> +			do_write_string(fd, name);
>> +			do_write(fd, &leader_idx, sizeof(leader_idx));
>> +			do_write(fd, &nr_members, sizeof(nr_members));
>
> You need to check do_write() return, it can fail. There are some cases
> in header.c that don't check it, those should be eventually fixed, most
> cases check it tho.

Okay.

>
>> +		}
>> +	}
>> +	return 0;
>> +}
[snip]
>> +static int process_group_desc(struct perf_file_section *section __maybe_unused,
>> +			      struct perf_header *ph, int fd,
>> +			      void *data __maybe_unused)
>> +{
>> +	size_t ret = -1;
>> +	u32 i, nr, nr_groups;
>> +	struct perf_session *session;
>> +	struct perf_evsel *evsel, *leader = NULL;
>> +	struct group_desc {
>> +		char *name;
>> +		u32 leader_idx;
>> +		u32 nr_members;
>> +	} *desc;
>> +
>> +	if (read(fd, &nr_groups, sizeof(nr_groups)) != sizeof(nr_groups))
>
> Please use readn() instead in all read() cases in this function.

Will do.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists
  2012-11-29 18:52   ` Arnaldo Carvalho de Melo
@ 2012-12-03  1:41     ` Namhyung Kim
  2012-12-03 10:19       ` Jiri Olsa
  0 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03  1:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML, Jiri Olsa,
	Stephane Eranian, Andi Kleen, Namhyung Kim

On Thu, 29 Nov 2012 15:52:57 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 29, 2012 at 03:38:35PM +0900, Namhyung Kim escreveu:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> When comparing entries for collapsing put the given entry first, and
>> then the iterated entry.  This is for the sake of consistency and will
>
> consistency with what? and, see below:
>
>> be required by the event group report.
>> 
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Stephane Eranian <eranian@google.com>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/util/hist.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
>> index 82df1b26f0d4..161c35e7ed0e 100644
>> --- a/tools/perf/util/hist.c
>> +++ b/tools/perf/util/hist.c
>> @@ -433,7 +433,7 @@ static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
>>  		parent = *p;
>>  		iter = rb_entry(parent, struct hist_entry, rb_node_in);
>>  
>> -		cmp = hist_entry__collapse(iter, he);
>> +		cmp = hist_entry__collapse(he, iter);
>>  
>>  		if (!cmp) {
>>  			he_stat__add_stat(&iter->stat, &he->stat);
>
> doesn't this now gets inconsistent with the hist_entry__collapse() call?
> I.e. iter first, he after, also there is the case for callchains, below,
> care to elaborate here?

I meant it by consistent with hist_entry__cmp() and didn't consider
he_stat__add_stat and callchain_merge things - thought that they're
other kind of operation.

I needed this change because I introduced hists__{match,link}_collapsed
function in the patch 8 and I found that hist_entry__cmp and
hist_entry__collapse received same kind of arguments in different
order.  Sorry about missing this in the changelog.

However on the second thought, I feel like I don't need those _collapsed
functions at all and perf diff can be converted to use collapsed rb tree
directly instead.  IIUC perf diff use those functions to match entries
by sort keys and do additional resort output rb tree by sort keys (IMHO
the function names - _name_resort and _insert_by_name - are misnomers)
to do the match.

Since output resorting (by period) is only needed for the baseline,
other data files doesn't need to do this additional step.  So I can get
rid of those hists__{match,link}_collapsed functions and change plain
hists__{match,link} functions to use collapsed (or input) rb tree
directly.

Jiri, what do you think?  What am I missing? :)

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-11-30 13:52     ` Arnaldo Carvalho de Melo
@ 2012-12-03  1:51       ` Namhyung Kim
  2012-12-04  9:16       ` Namhyung Kim
  1 sibling, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03  1:51 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML,
	Stephane Eranian, Andi Kleen, Namhyung Kim

On Fri, 30 Nov 2012 10:52:15 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 30, 2012 at 02:29:43PM +0100, Jiri Olsa escreveu:
>> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
>> > +#define __HPP_COLOR_PERCENT_FN(_type, _field)					\
>> > +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he) 	\
>> > +{										\
>> > +	int ret;								\
>> > +	double percent = 0.0;							\
>> > +	struct hists *hists = he->hists;					\
>> > +										\
>> > +	if (hists->stats.total_period)						\
>> > +		percent = 100.0 * he->stat._field / hists->stats.total_period; 	\
>> > +										\
>> > +	ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent);	\
>> > +										\
>> > +	if (symbol_conf.event_group) {						\
>> > +		int i;								\
>> > +		struct perf_evsel *evsel = hists_to_evsel(hists);		\
>> > +		struct hist_entry *pair;					\
>> > +		int nr_members = evsel->nr_members;				\
>> > +		double *percents;						\
>> > +										\
>> > +		if (nr_members <= 1)						\
>> > +			return ret;						\
>> > +										\
>> > +		percents = zalloc(sizeof(*percents) * nr_members);		\
>> > +		if (percents == NULL) {						\
>> > +			pr_warning("Not enough memory!\n");			\
>> > +			return ret;						\
>> > +		}								\
>
> Why do we need to zalloc this percents array?
>
>> > +		list_for_each_entry(pair, &he->pairs.head, pairs.node) { 	\
>> > +			u64 period = pair->stat._field;				\
>> > +			u64 total = pair->hists->stats.total_period;		\
>> > +										\
>> > +			if (!total)						\
>> > +				continue;					\
>> > +										\
>> > +			evsel = hists_to_evsel(pair->hists);			\
>> > +			i = perf_evsel__group_idx(evsel);			\
>> > +			percents[i] = 100.0 * period / total;			\
>
> Why not use percent_color_snprintf here, using some "%*s" thing that
> uses i to position the output in the right column? This way the
> following loop can be ditched as well. No?

It's because it's possible that an entry didn't have pairs for every
group member.  Say, there's a group consists of 3 members (1 leader + 2
member).  It's quite legitimate that a leader hist entry has just one
pair for a member and miss another.  So I allocated a zero-filled array,
and filled what it has, and print them all in the for loop below.

Thanks,
Namhyung

>
>> > +		}								\
>> > +										\
>> > +		for (i = 1; i < nr_members; i++) {				\
>> > +			ret += percent_color_snprintf(hpp->buf + ret,		\
>> > +						      hpp->size - ret,		\
>> > +						      " %6.2f%%", percents[i]);	\
>> > +		}								\
>> > +		free(percents);							\
>> > +	}									\
>> > +	return ret;								\
>
> - Arnaldo

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
       [not found]   ` <20121130132943.GC1080@krava.brq.redhat.com>
  2012-11-30 13:52     ` Arnaldo Carvalho de Melo
@ 2012-12-03  1:56     ` Namhyung Kim
  2012-12-03 10:23       ` Jiri Olsa
  1 sibling, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03  1:56 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

Hi Jiri,

On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>> +#define __HPP_COLOR_PERCENT_FN(_type, _field)                                        \
>> +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he)   \
>> +{                                                                            \
>> +     int ret;                                                                \
>> +     double percent = 0.0;                                                   \
>> +     struct hists *hists = he->hists;                                        \
>> +                                                                             \
>> +     if (hists->stats.total_period)                                          \
>> +             percent = 100.0 * he->stat._field / hists->stats.total_period;  \
>> +                                                                             \
>> +     ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent); \
>> +                                                                             \
>> +     if (symbol_conf.event_group) {                                          \
>> +             int i;                                                          \
>> +             struct perf_evsel *evsel = hists_to_evsel(hists);               \
>> +             struct hist_entry *pair;                                        \
>> +             int nr_members = evsel->nr_members;                             \
>> +             double *percents;                                               \
>> +                                                                             \
>> +             if (nr_members <= 1)                                            \
>> +                     return ret;                                             \
>> +                                                                             \
>> +             percents = zalloc(sizeof(*percents) * nr_members);              \
>> +             if (percents == NULL) {                                         \
>> +                     pr_warning("Not enough memory!\n");                     \
>> +                     return ret;                                             \
>> +             }                                                               \
>> +                                                                             \
>> +             list_for_each_entry(pair, &he->pairs.head, pairs.node) {        \
>> +                     u64 period = pair->stat._field;                         \
>> +                     u64 total = pair->hists->stats.total_period;            \
>> +                                                                             \
>> +                     if (!total)                                             \
>> +                             continue;                                       \
>> +                                                                             \
>> +                     evsel = hists_to_evsel(pair->hists);                    \
>> +                     i = perf_evsel__group_idx(evsel);                       \
>> +                     percents[i] = 100.0 * period / total;                   \
>> +             }                                                               \
>> +                                                                             \
>> +             for (i = 1; i < nr_members; i++) {                              \
>> +                     ret += percent_color_snprintf(hpp->buf + ret,           \
>> +                                                   hpp->size - ret,          \
>> +                                                   " %6.2f%%", percents[i]); \
>> +             }                                                               \
>> +             free(percents);                                                 \
>> +     }                                                                       \
>> +     return ret;                                                             \
>> +}
>
> ok, so this is the part thats common for both multi diff and group
> report and hugely depends on how we link matching hist_entry
>
> To sum to what group report does here:
>
> 1) starting with event group
> 2) the function 'he' belongs to leader hists
> 3) display leaders data value
> 4) if 'symbol_conf.event_group' is enabled, we want to display all
>    group members data values in a column
> 5) say we have group of 3 events 'e1,e2,e3' and e1 being the leader,
>    we have following possibilities:
>    - e1 have no pairs
>    - e1 is paired with e2
>    - e1 is paired with e3
>    - e1 is paired with e2 and e3 (e1 could also be dummy one)
> 6) need to display 3 values wrt to e1,e2,e3 output order
> 7) because events belongs to a group, each evsel carries group idx
> 8) so we walk all pairs and compute the eX value and put it
>    into temporary array based on its group idx
> 9) finally we display all temporary array values
>
>
> To sum up what multi diff needs to do here:
>
> 1) starting with 3 separate matching events from different
>    evlists(sessions)
> 2 - 5) are similar
> 6) need to display single diff value of hist_entry that
>    belongs to evsel, that belongs to a column we are just
>    displaying value for
> 7) events are not part of group; based on
>    [PATCH 02/14] perf tool: Add struct perf_hpp_fmt into hpp callbacks
>    we can tell what column we are in -> session -> evlist
> 8) we need to walk all pairs and for each hist_entry:
>    - compare all evsels (from point 7 evlist)
>      and match hists pointer
>    - when found, we have a matching hist_entry for this column
> 9) print out the value of matching hist_entry
>
>
> I think both this processing could be simplified by having hist_entry
> pairs connected via array and not linked list.
>
>
> For group report, each leader hist_entry would have 'pairs' array
> with the size of event group. Then we could omit the temporary array
> creation by:
>   - walking the leaders pairs
>   - when pair is found -> compute data -> display
>   - when pair is NULL  -> display 0
>
>
> For multi diff, each baseline hist_entry would have 'pairs' array
> with the size equal to number of data files on diff command input.
> This way we could use the data from point 7 to directly access
> related hist_entry.
>
>
> ufff... thoughts? ;-)

Nice summary, really! :)

Yeah, I do think it's better to use array for this.  If Arnaldo has no
objection to this approach, I'll convert my code to use the array.

And please see my another post for thoughts on the hists__{link,match} too. ;-)

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists
  2012-12-03  1:41     ` Namhyung Kim
@ 2012-12-03 10:19       ` Jiri Olsa
  2012-12-03 10:49         ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Jiri Olsa @ 2012-12-03 10:19 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Mon, Dec 03, 2012 at 10:41:08AM +0900, Namhyung Kim wrote:
> On Thu, 29 Nov 2012 15:52:57 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Nov 29, 2012 at 03:38:35PM +0900, Namhyung Kim escreveu:
> >> From: Namhyung Kim <namhyung.kim@lge.com>
> >> 
> >> When comparing entries for collapsing put the given entry first, and
> >> then the iterated entry.  This is for the sake of consistency and will
> >
> > consistency with what? and, see below:
> >
> >> be required by the event group report.
> >> 
> >> Cc: Jiri Olsa <jolsa@redhat.com>
> >> Cc: Stephane Eranian <eranian@google.com>
> >> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >> ---
> >>  tools/perf/util/hist.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> >> index 82df1b26f0d4..161c35e7ed0e 100644
> >> --- a/tools/perf/util/hist.c
> >> +++ b/tools/perf/util/hist.c
> >> @@ -433,7 +433,7 @@ static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
> >>  		parent = *p;
> >>  		iter = rb_entry(parent, struct hist_entry, rb_node_in);
> >>  
> >> -		cmp = hist_entry__collapse(iter, he);
> >> +		cmp = hist_entry__collapse(he, iter);
> >>  
> >>  		if (!cmp) {
> >>  			he_stat__add_stat(&iter->stat, &he->stat);
> >
> > doesn't this now gets inconsistent with the hist_entry__collapse() call?
> > I.e. iter first, he after, also there is the case for callchains, below,
> > care to elaborate here?
> 
> I meant it by consistent with hist_entry__cmp() and didn't consider
> he_stat__add_stat and callchain_merge things - thought that they're
> other kind of operation.
> 
> I needed this change because I introduced hists__{match,link}_collapsed
> function in the patch 8 and I found that hist_entry__cmp and
> hist_entry__collapse received same kind of arguments in different
> order.  Sorry about missing this in the changelog.
> 
> However on the second thought, I feel like I don't need those _collapsed
> functions at all and perf diff can be converted to use collapsed rb tree
> directly instead.  IIUC perf diff use those functions to match entries
> by sort keys and do additional resort output rb tree by sort keys (IMHO
> the function names - _name_resort and _insert_by_name - are misnomers)
> to do the match.
> 
> Since output resorting (by period) is only needed for the baseline,
> other data files doesn't need to do this additional step.  So I can get
> rid of those hists__{match,link}_collapsed functions and change plain
> hists__{match,link} functions to use collapsed (or input) rb tree
> directly.
>
> Jiri, what do you think?  What am I missing? :)

feels like I'm missing something now :))

right now I don't see a point in having sort__need_collapse enabled
for group report.. there seems to be no special processing as it was
in the initial patchset..?

It seems like you could do only stadard period resort (collapsed if
needed) and afterwards run current hists__match and hists_link function
they are now.

All you need at the output callback is paired leader hist entry.. where
you do the temp array magic to get the data.

/me going to dishonour some whiteboard.. ;)

jirka

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-12-03  1:56     ` Namhyung Kim
@ 2012-12-03 10:23       ` Jiri Olsa
  2012-12-03 10:39         ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Jiri Olsa @ 2012-12-03 10:23 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
> >
> > SNIP
> >
> >> +#define __HPP_COLOR_PERCENT_FN(_type, _field)                                        \
> >> +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he)   \
> >> +{                                                                            \
> >> +     int ret;                                                                \
> >> +     double percent = 0.0;                                                   \
> >> +     struct hists *hists = he->hists;                                        \
> >> +                                                                             \
> >> +     if (hists->stats.total_period)                                          \
> >> +             percent = 100.0 * he->stat._field / hists->stats.total_period;  \
> >> +                                                                             \
> >> +     ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent); \
> >> +                                                                             \
> >> +     if (symbol_conf.event_group) {                                          \
> >> +             int i;                                                          \
> >> +             struct perf_evsel *evsel = hists_to_evsel(hists);               \
> >> +             struct hist_entry *pair;                                        \
> >> +             int nr_members = evsel->nr_members;                             \
> >> +             double *percents;                                               \
> >> +                                                                             \
> >> +             if (nr_members <= 1)                                            \
> >> +                     return ret;                                             \
> >> +                                                                             \
> >> +             percents = zalloc(sizeof(*percents) * nr_members);              \
> >> +             if (percents == NULL) {                                         \
> >> +                     pr_warning("Not enough memory!\n");                     \
> >> +                     return ret;                                             \
> >> +             }                                                               \
> >> +                                                                             \
> >> +             list_for_each_entry(pair, &he->pairs.head, pairs.node) {        \
> >> +                     u64 period = pair->stat._field;                         \
> >> +                     u64 total = pair->hists->stats.total_period;            \
> >> +                                                                             \
> >> +                     if (!total)                                             \
> >> +                             continue;                                       \
> >> +                                                                             \
> >> +                     evsel = hists_to_evsel(pair->hists);                    \
> >> +                     i = perf_evsel__group_idx(evsel);                       \
> >> +                     percents[i] = 100.0 * period / total;                   \
> >> +             }                                                               \
> >> +                                                                             \
> >> +             for (i = 1; i < nr_members; i++) {                              \
> >> +                     ret += percent_color_snprintf(hpp->buf + ret,           \
> >> +                                                   hpp->size - ret,          \
> >> +                                                   " %6.2f%%", percents[i]); \
> >> +             }                                                               \
> >> +             free(percents);                                                 \
> >> +     }                                                                       \
> >> +     return ret;                                                             \
> >> +}
> >
> > ok, so this is the part thats common for both multi diff and group
> > report and hugely depends on how we link matching hist_entry
> >
> > To sum to what group report does here:
> >
> > 1) starting with event group
> > 2) the function 'he' belongs to leader hists
> > 3) display leaders data value
> > 4) if 'symbol_conf.event_group' is enabled, we want to display all
> >    group members data values in a column
> > 5) say we have group of 3 events 'e1,e2,e3' and e1 being the leader,
> >    we have following possibilities:
> >    - e1 have no pairs
> >    - e1 is paired with e2
> >    - e1 is paired with e3
> >    - e1 is paired with e2 and e3 (e1 could also be dummy one)
> > 6) need to display 3 values wrt to e1,e2,e3 output order
> > 7) because events belongs to a group, each evsel carries group idx
> > 8) so we walk all pairs and compute the eX value and put it
> >    into temporary array based on its group idx
> > 9) finally we display all temporary array values
> >
> >
> > To sum up what multi diff needs to do here:
> >
> > 1) starting with 3 separate matching events from different
> >    evlists(sessions)
> > 2 - 5) are similar
> > 6) need to display single diff value of hist_entry that
> >    belongs to evsel, that belongs to a column we are just
> >    displaying value for
> > 7) events are not part of group; based on
> >    [PATCH 02/14] perf tool: Add struct perf_hpp_fmt into hpp callbacks
> >    we can tell what column we are in -> session -> evlist
> > 8) we need to walk all pairs and for each hist_entry:
> >    - compare all evsels (from point 7 evlist)
> >      and match hists pointer
> >    - when found, we have a matching hist_entry for this column
> > 9) print out the value of matching hist_entry
> >
> >
> > I think both this processing could be simplified by having hist_entry
> > pairs connected via array and not linked list.
> >
> >
> > For group report, each leader hist_entry would have 'pairs' array
> > with the size of event group. Then we could omit the temporary array
> > creation by:
> >   - walking the leaders pairs
> >   - when pair is found -> compute data -> display
> >   - when pair is NULL  -> display 0
> >
> >
> > For multi diff, each baseline hist_entry would have 'pairs' array
> > with the size equal to number of data files on diff command input.
> > This way we could use the data from point 7 to directly access
> > related hist_entry.
> >
> >
> > ufff... thoughts? ;-)
> 
> Nice summary, really! :)
> 
> Yeah, I do think it's better to use array for this.  If Arnaldo has no
> objection to this approach, I'll convert my code to use the array.

we discussed with Arnaldo and decided to stay with current approach and
make the change later if needed.. to be able to see/meassure the benefit

I made some initial attempt to workaround this and it appears to be
not that bad ;) I'll repost my changes shortly..

> 
> And please see my another post for thoughts on the hists__{link,match} too. ;-)

yep, answered ;)

thanks,
jirka

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-12-03 10:23       ` Jiri Olsa
@ 2012-12-03 10:39         ` Namhyung Kim
  2012-12-03 15:57           ` Jiri Olsa
  0 siblings, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03 10:39 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Mon, 3 Dec 2012 11:23:27 +0100, Jiri Olsa wrote:
> On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote:
>> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>> > ok, so this is the part thats common for both multi diff and group
>> > report and hugely depends on how we link matching hist_entry
>> >
>> > To sum to what group report does here:
>> >
>> > 1) starting with event group
>> > 2) the function 'he' belongs to leader hists
>> > 3) display leaders data value
>> > 4) if 'symbol_conf.event_group' is enabled, we want to display all
>> >    group members data values in a column
>> > 5) say we have group of 3 events 'e1,e2,e3' and e1 being the leader,
>> >    we have following possibilities:
>> >    - e1 have no pairs
>> >    - e1 is paired with e2
>> >    - e1 is paired with e3
>> >    - e1 is paired with e2 and e3 (e1 could also be dummy one)
>> > 6) need to display 3 values wrt to e1,e2,e3 output order
>> > 7) because events belongs to a group, each evsel carries group idx
>> > 8) so we walk all pairs and compute the eX value and put it
>> >    into temporary array based on its group idx
>> > 9) finally we display all temporary array values
>> >
>> >
>> > To sum up what multi diff needs to do here:
>> >
>> > 1) starting with 3 separate matching events from different
>> >    evlists(sessions)
>> > 2 - 5) are similar
>> > 6) need to display single diff value of hist_entry that
>> >    belongs to evsel, that belongs to a column we are just
>> >    displaying value for
>> > 7) events are not part of group; based on
>> >    [PATCH 02/14] perf tool: Add struct perf_hpp_fmt into hpp callbacks
>> >    we can tell what column we are in -> session -> evlist
>> > 8) we need to walk all pairs and for each hist_entry:
>> >    - compare all evsels (from point 7 evlist)
>> >      and match hists pointer
>> >    - when found, we have a matching hist_entry for this column
>> > 9) print out the value of matching hist_entry
>> >
>> >
>> > I think both this processing could be simplified by having hist_entry
>> > pairs connected via array and not linked list.
>> >
>> >
>> > For group report, each leader hist_entry would have 'pairs' array
>> > with the size of event group. Then we could omit the temporary array
>> > creation by:
>> >   - walking the leaders pairs
>> >   - when pair is found -> compute data -> display
>> >   - when pair is NULL  -> display 0
>> >
>> >
>> > For multi diff, each baseline hist_entry would have 'pairs' array
>> > with the size equal to number of data files on diff command input.
>> > This way we could use the data from point 7 to directly access
>> > related hist_entry.
>> >
>> >
>> > ufff... thoughts? ;-)
>> 
>> Nice summary, really! :)
>> 
>> Yeah, I do think it's better to use array for this.  If Arnaldo has no
>> objection to this approach, I'll convert my code to use the array.
>
> we discussed with Arnaldo and decided to stay with current approach and
> make the change later if needed.. to be able to see/meassure the benefit
>
> I made some initial attempt to workaround this and it appears to be
> not that bad ;) I'll repost my changes shortly..

Hmm.. so are you OK with this patch now?

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists
  2012-12-03 10:19       ` Jiri Olsa
@ 2012-12-03 10:49         ` Namhyung Kim
  0 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03 10:49 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Mon, 3 Dec 2012 11:19:52 +0100, Jiri Olsa wrote:
> On Mon, Dec 03, 2012 at 10:41:08AM +0900, Namhyung Kim wrote:
>> On Thu, 29 Nov 2012 15:52:57 -0300, Arnaldo Carvalho de Melo wrote:
>> > doesn't this now gets inconsistent with the hist_entry__collapse() call?
>> > I.e. iter first, he after, also there is the case for callchains, below,
>> > care to elaborate here?
>> 
>> I meant it by consistent with hist_entry__cmp() and didn't consider
>> he_stat__add_stat and callchain_merge things - thought that they're
>> other kind of operation.
>> 
>> I needed this change because I introduced hists__{match,link}_collapsed
>> function in the patch 8 and I found that hist_entry__cmp and
>> hist_entry__collapse received same kind of arguments in different
>> order.  Sorry about missing this in the changelog.
>> 
>> However on the second thought, I feel like I don't need those _collapsed
>> functions at all and perf diff can be converted to use collapsed rb tree
>> directly instead.  IIUC perf diff use those functions to match entries
>> by sort keys and do additional resort output rb tree by sort keys (IMHO
>> the function names - _name_resort and _insert_by_name - are misnomers)
>> to do the match.
>> 
>> Since output resorting (by period) is only needed for the baseline,
>> other data files doesn't need to do this additional step.  So I can get
>> rid of those hists__{match,link}_collapsed functions and change plain
>> hists__{match,link} functions to use collapsed (or input) rb tree
>> directly.
>>
>> Jiri, what do you think?  What am I missing? :)
>
> feels like I'm missing something now :))
>
> right now I don't see a point in having sort__need_collapse enabled
> for group report.. there seems to be no special processing as it was
> in the initial patchset..?

Right.  It's not needed anymore, will remove.

>
> It seems like you could do only stadard period resort (collapsed if
> needed) and afterwards run current hists__match and hists_link function
> they are now.

But hists__match/link needs the tree sorted by sort keys.  That means I
have to do something like:

(1) add_hist_entry (sort by keys)
(2) (optionally) hists__collapse_resort (sort by keys)
(3) hists__output_resort (sort by period)
(4) manual resort (by keys)
(5) hists__match/link
(6) (duplicated) hists__output_resort (sort by period)

I think the step (3) and (4) are unnecessary if we use input (or
collapsed) tree directly.

>
> All you need at the output callback is paired leader hist entry.. where
> you do the temp array magic to get the data.

Yeah, this is what I'm saying.  So we don't need to use output tree for
matching and linking entries.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-12-03 10:39         ` Namhyung Kim
@ 2012-12-03 15:57           ` Jiri Olsa
  2012-12-03 16:19             ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Jiri Olsa @ 2012-12-03 15:57 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Mon, Dec 03, 2012 at 07:39:31PM +0900, Namhyung Kim wrote:
> On Mon, 3 Dec 2012 11:23:27 +0100, Jiri Olsa wrote:
> > On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote:
> >> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> >> > ok, so this is the part thats common for both multi diff and group
> >> > report and hugely depends on how we link matching hist_entry

SNIP

> > we discussed with Arnaldo and decided to stay with current approach and
> > make the change later if needed.. to be able to see/meassure the benefit
> >
> > I made some initial attempt to workaround this and it appears to be
> > not that bad ;) I'll repost my changes shortly..
> 
> Hmm.. so are you OK with this patch now?

yep, well except for following macros:

	__HPP_COLOR_PERCENT_FN
	__HPP_ENTRY_PERCENT_FN
	__HPP_ENTRY_RAW_FN

being so long.. any chance the main part of it being in function?

Seems like '_type' is just in function name, but the '_field' might
be the killer ;) maybe it could be 'used' in such function via PERF_HPP__*
enums instead..?

thanks,
jirka

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-12-03 15:57           ` Jiri Olsa
@ 2012-12-03 16:19             ` Namhyung Kim
  0 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-12-03 16:19 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, LKML, Stephane Eranian, Andi Kleen, Namhyung Kim

On Mon, 3 Dec 2012 16:57:36 +0100, Jiri Olsa wrote:
> On Mon, Dec 03, 2012 at 07:39:31PM +0900, Namhyung Kim wrote:
>> On Mon, 3 Dec 2012 11:23:27 +0100, Jiri Olsa wrote:
>> > On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote:
>> >> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>> >> > ok, so this is the part thats common for both multi diff and group
>> >> > report and hugely depends on how we link matching hist_entry
>
> SNIP
>
>> > we discussed with Arnaldo and decided to stay with current approach and
>> > make the change later if needed.. to be able to see/meassure the benefit
>> >
>> > I made some initial attempt to workaround this and it appears to be
>> > not that bad ;) I'll repost my changes shortly..
>> 
>> Hmm.. so are you OK with this patch now?
>
> yep, well except for following macros:
>
> 	__HPP_COLOR_PERCENT_FN
> 	__HPP_ENTRY_PERCENT_FN
> 	__HPP_ENTRY_RAW_FN
>
> being so long.. any chance the main part of it being in function?
>
> Seems like '_type' is just in function name, but the '_field' might
> be the killer ;) maybe it could be 'used' in such function via PERF_HPP__*
> enums instead..?

Okay, I'll re-think about it tomorrow.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-11-30 13:52     ` Arnaldo Carvalho de Melo
  2012-12-03  1:51       ` Namhyung Kim
@ 2012-12-04  9:16       ` Namhyung Kim
  2012-12-04 13:50         ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 50+ messages in thread
From: Namhyung Kim @ 2012-12-04  9:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML,
	Stephane Eranian, Andi Kleen, Namhyung Kim

On Fri, Nov 30, 2012 at 10:52 PM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> wrote:
> Em Fri, Nov 30, 2012 at 02:29:43PM +0100, Jiri Olsa escreveu:
>> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
>> > +#define __HPP_COLOR_PERCENT_FN(_type, _field)                                      \
>> > +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he)         \
>> > +{                                                                          \
>> > +   int ret;                                                                \
>> > +   double percent = 0.0;                                                   \
>> > +   struct hists *hists = he->hists;                                        \
>> > +                                                                           \
>> > +   if (hists->stats.total_period)                                          \
>> > +           percent = 100.0 * he->stat._field / hists->stats.total_period;  \
>> > +                                                                           \
>> > +   ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent); \
>> > +                                                                           \
>> > +   if (symbol_conf.event_group) {                                          \
>> > +           int i;                                                          \
>> > +           struct perf_evsel *evsel = hists_to_evsel(hists);               \
>> > +           struct hist_entry *pair;                                        \
>> > +           int nr_members = evsel->nr_members;                             \
>> > +           double *percents;                                               \
>> > +                                                                           \
>> > +           if (nr_members <= 1)                                            \
>> > +                   return ret;                                             \
>> > +                                                                           \
>> > +           percents = zalloc(sizeof(*percents) * nr_members);              \
>> > +           if (percents == NULL) {                                         \
>> > +                   pr_warning("Not enough memory!\n");                     \
>> > +                   return ret;                                             \
>> > +           }                                                               \
>
> Why do we need to zalloc this percents array?
>
>> > +           list_for_each_entry(pair, &he->pairs.head, pairs.node) {        \
>> > +                   u64 period = pair->stat._field;                         \
>> > +                   u64 total = pair->hists->stats.total_period;            \
>> > +                                                                           \
>> > +                   if (!total)                                             \
>> > +                           continue;                                       \
>> > +                                                                           \
>> > +                   evsel = hists_to_evsel(pair->hists);                    \
>> > +                   i = perf_evsel__group_idx(evsel);                       \
>> > +                   percents[i] = 100.0 * period / total;                   \
>
> Why not use percent_color_snprintf here, using some "%*s" thing that
> uses i to position the output in the right column? This way the
> following loop can be ditched as well. No?
>
>> > +           }                                                               \
>> > +                                                                           \
>> > +           for (i = 1; i < nr_members; i++) {                              \
>> > +                   ret += percent_color_snprintf(hpp->buf + ret,           \
>> > +                                                 hpp->size - ret,          \
>> > +                                                 " %6.2f%%", percents[i]); \
>> > +           }                                                               \
>> > +           free(percents);                                                 \
>> > +   }                                                                       \
>> > +   return ret;                                                             \
>
> - Arnaldo

Ah, I missed your point.  Just got it now, will try this approach.  So
you want to see no "0.00%" for a dummy entry, right?

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-12-04  9:16       ` Namhyung Kim
@ 2012-12-04 13:50         ` Arnaldo Carvalho de Melo
  2012-12-04 14:51           ` Namhyung Kim
  0 siblings, 1 reply; 50+ messages in thread
From: Arnaldo Carvalho de Melo @ 2012-12-04 13:50 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Jiri Olsa, Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML,
	Stephane Eranian, Andi Kleen, Namhyung Kim

Em Tue, Dec 04, 2012 at 06:16:59PM +0900, Namhyung Kim escreveu:
> On Fri, Nov 30, 2012 at 10:52 PM, Arnaldo Carvalho de Melo wrote:

> Ah, I missed your point.  Just got it now, will try this approach.  So
> you want to see no "0.00%" for a dummy entry, right?

That wasn't the point, and perhaps printing 0.00% pollutes the screen
needlessly or may be a help in seeing the columns more clearly, no
strong opinion at this point, please experiment.

The point was that there seems to be no need for the temporary array.

- Arnaldo

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 12/18] perf ui/hist: Add support for event group view
  2012-12-04 13:50         ` Arnaldo Carvalho de Melo
@ 2012-12-04 14:51           ` Namhyung Kim
  0 siblings, 0 replies; 50+ messages in thread
From: Namhyung Kim @ 2012-12-04 14:51 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Peter Zijlstra, Paul Mackerras, Ingo Molnar, LKML,
	Stephane Eranian, Andi Kleen, Namhyung Kim

Hi Arnaldo,

On Tue, 4 Dec 2012 10:50:49 -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 04, 2012 at 06:16:59PM +0900, Namhyung Kim escreveu:
>> On Fri, Nov 30, 2012 at 10:52 PM, Arnaldo Carvalho de Melo wrote:
>
>> Ah, I missed your point.  Just got it now, will try this approach.  So
>> you want to see no "0.00%" for a dummy entry, right?
>
> That wasn't the point, and perhaps printing 0.00% pollutes the screen
> needlessly or may be a help in seeing the columns more clearly, no
> strong opinion at this point, please experiment.
>
> The point was that there seems to be no need for the temporary array.

Yeah, it shows my really bad writing skill. :(

I meant I got your point of not using a temporary array.  And asked
additional question of your preference in a dummy entry.

Sorry for confusing you.

Btw, I found that printing 0.00% is useful for a case of field_sep and
GTK+ output.  So unless someone objects, I'll keep it.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [tip:perf/core] perf hists: Link hist entry pairs to leader
  2012-11-29  6:38 ` [PATCH 06/18] perf hists: Link hist entry pairs to leader Namhyung Kim
@ 2013-01-24 18:47   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 50+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-01-24 18:47 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, paulus, hpa, mingo, andi,
	a.p.zijlstra, namhyung.kim, namhyung, jolsa, tglx

Commit-ID:  5fa9041bbaa7a79a67d568b9c9f947db2f23d091
Gitweb:     http://git.kernel.org/tip/5fa9041bbaa7a79a67d568b9c9f947db2f23d091
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Thu, 29 Nov 2012 15:38:34 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Sun, 9 Dec 2012 08:46:06 -0300

perf hists: Link hist entry pairs to leader

Current hists__match/link() link a leader to its pair, so if multiple
pairs were linked, the leader will lose pointer to previous pairs since
it was overwritten.  Fix it by making leader the list head.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1354171126-14387-8-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index d2bc05c..82df1b2 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -785,7 +785,7 @@ void hists__match(struct hists *leader, struct hists *other)
 		pair = hists__find_entry(other, pos);
 
 		if (pair)
-			hist_entry__add_pair(pos, pair);
+			hist_entry__add_pair(pair, pos);
 	}
 }
 
@@ -806,7 +806,7 @@ int hists__link(struct hists *leader, struct hists *other)
 			pair = hists__add_dummy_entry(leader, pos);
 			if (pair == NULL)
 				return -1;
-			hist_entry__add_pair(pair, pos);
+			hist_entry__add_pair(pos, pair);
 		}
 	}
 

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [tip:perf/core] perf evsel: Set leader evsel's ->leader to itself
  2012-11-29  6:38 ` [PATCH 01/18] perf evsel: Set leader evsel's ->leader to itself Namhyung Kim
  2012-11-29 14:28   ` Jiri Olsa
@ 2013-01-24 18:48   ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 50+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-01-24 18:48 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, paulus, hpa, mingo, andi,
	a.p.zijlstra, acme, namhyung.kim, namhyung, jolsa, tglx

Commit-ID:  2cfda562da7b0b1e0575507ef3efe782d4e218e4
Gitweb:     http://git.kernel.org/tip/2cfda562da7b0b1e0575507ef3efe782d4e218e4
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Thu, 29 Nov 2012 15:38:29 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Sun, 9 Dec 2012 08:46:06 -0300

perf evsel: Set leader evsel's ->leader to itself

Currently only non-leader members are set ->leader to the leader evsel
of the group and the leader has set NULL.  Thus it requires special
casing for leader evsels.  Set ->leader to itself will remove this.

Suggested-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1354171126-14387-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 1 -
 tools/perf/util/evsel.c  | 1 +
 tools/perf/util/evsel.h  | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7052934..90db2a1 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -111,7 +111,6 @@ void __perf_evlist__set_leader(struct list_head *list)
 	struct perf_evsel *evsel, *leader;
 
 	leader = list_entry(list->next, struct perf_evsel, node);
-	leader->leader = NULL;
 
 	list_for_each_entry(evsel, list, node) {
 		if (evsel != leader)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1b16dd1..7e93418 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -55,6 +55,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
 {
 	evsel->idx	   = idx;
 	evsel->attr	   = *attr;
+	evsel->leader	   = evsel;
 	INIT_LIST_HEAD(&evsel->node);
 	hists__init(&evsel->hists);
 	evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 3d2b801..cbf6d97 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -228,6 +228,6 @@ static inline struct perf_evsel *perf_evsel__next(struct perf_evsel *evsel)
 
 static inline bool perf_evsel__is_group_member(const struct perf_evsel *evsel)
 {
-	return evsel->leader != NULL;
+	return evsel->leader != evsel;
 }
 #endif /* __PERF_EVSEL_H */

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [tip:perf/core] perf evsel: Convert to _is_group_leader method
  2012-11-29  6:38 ` [PATCH 02/18] perf evsel: Convert to _is_group_leader method Namhyung Kim
  2012-11-29 14:28   ` Jiri Olsa
@ 2013-01-24 18:49   ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 50+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-01-24 18:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, paulus, hpa, mingo, andi,
	a.p.zijlstra, namhyung.kim, namhyung, jolsa, tglx

Commit-ID:  823254edc66eb44bf612b1dfa4829afa9840f691
Gitweb:     http://git.kernel.org/tip/823254edc66eb44bf612b1dfa4829afa9840f691
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Thu, 29 Nov 2012 15:38:30 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Sun, 9 Dec 2012 08:46:06 -0300

perf evsel: Convert to _is_group_leader method

Convert perf_evsel__is_group_member to perf_evsel__is_group_leader.
This is because the most usecases are using negative form to check
whether the given evsel is a leader or not and it's IMHO somewhat
ambiguous - leader also *is* a member of the group.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1354171126-14387-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c       |  2 +-
 tools/perf/tests/parse-events.c | 20 ++++++++++----------
 tools/perf/util/evlist.c        |  4 ++--
 tools/perf/util/evsel.c         |  6 +++---
 tools/perf/util/evsel.h         |  4 ++--
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c247fac..c12655a 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -153,7 +153,7 @@ retry:
 	}
 
 	if (!perf_target__has_task(&target) &&
-	    !perf_evsel__is_group_member(evsel)) {
+	    perf_evsel__is_group_leader(evsel)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 32ee478..294ffdd 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -521,7 +521,7 @@ static int test__group1(struct perf_evlist *evlist)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* cycles:upp */
 	evsel = perf_evsel__next(evsel);
@@ -557,7 +557,7 @@ static int test__group2(struct perf_evlist *evlist)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* cache-references + :u modifier */
 	evsel = perf_evsel__next(evsel);
@@ -583,7 +583,7 @@ static int test__group2(struct perf_evlist *evlist)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	return 0;
 }
@@ -606,7 +606,7 @@ static int test__group3(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 	TEST_ASSERT_VAL("wrong group name",
 		!strcmp(leader->group_name, "group1"));
 
@@ -636,7 +636,7 @@ static int test__group3(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 	TEST_ASSERT_VAL("wrong group name",
 		!strcmp(leader->group_name, "group2"));
 
@@ -663,7 +663,7 @@ static int test__group3(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	return 0;
 }
@@ -687,7 +687,7 @@ static int test__group4(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", evsel->attr.precise_ip == 1);
 	TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* instructions:kp + p */
 	evsel = perf_evsel__next(evsel);
@@ -724,7 +724,7 @@ static int test__group5(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
 	TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* instructions + G */
 	evsel = perf_evsel__next(evsel);
@@ -751,7 +751,7 @@ static int test__group5(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
 	TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	/* instructions:G */
 	evsel = perf_evsel__next(evsel);
@@ -777,7 +777,7 @@ static int test__group5(struct perf_evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong exclude guest", evsel->attr.exclude_guest);
 	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
-	TEST_ASSERT_VAL("wrong leader", !perf_evsel__is_group_member(evsel));
+	TEST_ASSERT_VAL("wrong leader", perf_evsel__is_group_leader(evsel));
 
 	return 0;
 }
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 90db2a1..d0e1e82 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -221,7 +221,7 @@ void perf_evlist__disable(struct perf_evlist *evlist)
 
 	for (cpu = 0; cpu < evlist->cpus->nr; cpu++) {
 		list_for_each_entry(pos, &evlist->entries, node) {
-			if (perf_evsel__is_group_member(pos))
+			if (!perf_evsel__is_group_leader(pos))
 				continue;
 			for (thread = 0; thread < evlist->threads->nr; thread++)
 				ioctl(FD(pos, cpu, thread),
@@ -237,7 +237,7 @@ void perf_evlist__enable(struct perf_evlist *evlist)
 
 	for (cpu = 0; cpu < cpu_map__nr(evlist->cpus); cpu++) {
 		list_for_each_entry(pos, &evlist->entries, node) {
-			if (perf_evsel__is_group_member(pos))
+			if (!perf_evsel__is_group_leader(pos))
 				continue;
 			for (thread = 0; thread < evlist->threads->nr; thread++)
 				ioctl(FD(pos, cpu, thread),
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 7e93418..bb58b05 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -520,14 +520,14 @@ void perf_evsel__config(struct perf_evsel *evsel,
 	 * Disabling only independent events or group leaders,
 	 * keeping group members enabled.
 	 */
-	if (!perf_evsel__is_group_member(evsel))
+	if (perf_evsel__is_group_leader(evsel))
 		attr->disabled = 1;
 
 	/*
 	 * Setting enable_on_exec for independent events and
 	 * group leaders for traced executed by perf.
 	 */
-	if (perf_target__none(&opts->target) && !perf_evsel__is_group_member(evsel))
+	if (perf_target__none(&opts->target) && perf_evsel__is_group_leader(evsel))
 		attr->enable_on_exec = 1;
 }
 
@@ -708,7 +708,7 @@ static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
 	struct perf_evsel *leader = evsel->leader;
 	int fd;
 
-	if (!perf_evsel__is_group_member(evsel))
+	if (perf_evsel__is_group_leader(evsel))
 		return -1;
 
 	/*
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index cbf6d97..3f7ff47 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -226,8 +226,8 @@ static inline struct perf_evsel *perf_evsel__next(struct perf_evsel *evsel)
 	return list_entry(evsel->node.next, struct perf_evsel, node);
 }
 
-static inline bool perf_evsel__is_group_member(const struct perf_evsel *evsel)
+static inline bool perf_evsel__is_group_leader(const struct perf_evsel *evsel)
 {
-	return evsel->leader != evsel;
+	return evsel->leader == evsel;
 }
 #endif /* __PERF_EVSEL_H */

^ permalink raw reply related	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2013-01-24 18:50 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-29  6:38 [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim
2012-11-29  6:38 ` Namhyung Kim
2012-11-29 12:21   ` Jiri Olsa
2012-11-29  6:38 ` [PATCH 01/18] perf evsel: Set leader evsel's ->leader to itself Namhyung Kim
2012-11-29 14:28   ` Jiri Olsa
2013-01-24 18:48   ` [tip:perf/core] " tip-bot for Namhyung Kim
2012-11-29  6:38 ` [PATCH 02/18] perf evsel: Convert to _is_group_leader method Namhyung Kim
2012-11-29 14:28   ` Jiri Olsa
2013-01-24 18:49   ` [tip:perf/core] " tip-bot for Namhyung Kim
2012-11-29  6:38 ` [PATCH 03/18] perf tools: Keep group information Namhyung Kim
2012-11-29 14:33   ` Jiri Olsa
2012-11-29 14:58     ` Namhyung Kim
2012-11-29 15:02       ` Jiri Olsa
2012-11-29 15:09         ` Namhyung Kim
2012-11-29 19:09           ` Arnaldo Carvalho de Melo
2012-12-03  1:12             ` Namhyung Kim
2012-11-29  6:38 ` [PATCH 04/18] perf header: Add HEADER_GROUP_DESC feature Namhyung Kim
2012-11-29 18:43   ` Arnaldo Carvalho de Melo
2012-12-03  1:16     ` Namhyung Kim
2012-11-29  6:38 ` [PATCH 05/18] perf tools: Fix typo on hist__entry_add_pair Namhyung Kim
2012-11-29 14:33   ` Jiri Olsa
2012-11-29  6:38 ` [PATCH 06/18] perf hists: Link hist entry pairs to leader Namhyung Kim
2013-01-24 18:47   ` [tip:perf/core] " tip-bot for Namhyung Kim
2012-11-29  6:38 ` [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists Namhyung Kim
2012-11-29 18:52   ` Arnaldo Carvalho de Melo
2012-12-03  1:41     ` Namhyung Kim
2012-12-03 10:19       ` Jiri Olsa
2012-12-03 10:49         ` Namhyung Kim
2012-11-29  6:38 ` [PATCH 08/18] perf hists: Add hists__{match,link}_collapsed Namhyung Kim
2012-11-29  6:38 ` [PATCH 09/18] perf symbol: Introduce symbol_conf.event_group Namhyung Kim
2012-11-29  6:38 ` [PATCH 10/18] perf report: Make another loop for linking group hists Namhyung Kim
2012-11-29  6:38 ` [PATCH 11/18] perf hists: Resort hist entries using group members for output Namhyung Kim
2012-11-29  6:38 ` [PATCH 12/18] perf ui/hist: Add support for event group view Namhyung Kim
     [not found]   ` <20121130132943.GC1080@krava.brq.redhat.com>
2012-11-30 13:52     ` Arnaldo Carvalho de Melo
2012-12-03  1:51       ` Namhyung Kim
2012-12-04  9:16       ` Namhyung Kim
2012-12-04 13:50         ` Arnaldo Carvalho de Melo
2012-12-04 14:51           ` Namhyung Kim
2012-12-03  1:56     ` Namhyung Kim
2012-12-03 10:23       ` Jiri Olsa
2012-12-03 10:39         ` Namhyung Kim
2012-12-03 15:57           ` Jiri Olsa
2012-12-03 16:19             ` Namhyung Kim
2012-11-29  6:38 ` [PATCH 13/18] perf hist browser: " Namhyung Kim
2012-11-29  6:38 ` [PATCH 14/18] perf gtk/browser: " Namhyung Kim
2012-11-29  6:38 ` [PATCH 15/18] perf report: Bypass non-leader events when event group is enabled Namhyung Kim
2012-11-29  6:38 ` [PATCH 16/18] perf report: Show group description " Namhyung Kim
2012-11-29  6:38 ` [PATCH 17/18] perf report: Add --group option Namhyung Kim
2012-11-29  6:38 ` [PATCH 18/18] perf report: Add report.group config option Namhyung Kim
2012-11-29  6:44 ` [PATCH 00/18] perf report: Add support for event group view (v6) Namhyung Kim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.