[PATCHSET 00/12] perf report: Add support for event group view (v5)

[PATCHSET 00/12] perf report: Add support for event group view (v5)

[Namhyung Kim]

Hi,

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

This is mostly for rebasing on acme/perf/core, and report.group config
option is added for those who wants to enable it by default.

You can get this series via my tree at:

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

Any comments are welcome, thanks,
Namhyung


v4 -> v5:
 * rebase on top of current 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

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>

Namhyung Kim (12):
  perf tools: Keep group information
  perf header: Add HEADER_GROUP_DESC feature
  perf hists: Collapse group hist_entries to a leader
  perf hists: Maintain total periods of group members in the leader
  perf report: Make another loop for output resorting
  perf ui/hist: Add support for event group view
  perf ui/browser: Add support for event group view
  perf ui/gtk: 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/builtin-record.c    |    3 +
 tools/perf/builtin-report.c    |   42 +++++++++++
 tools/perf/ui/browsers/hists.c |  101 ++++++++++++++++++++++++--
 tools/perf/ui/gtk/browser.c    |   72 +++++++++++++++----
 tools/perf/ui/hist.c           |   71 +++++++++++++++++--
 tools/perf/ui/stdio/hist.c     |    2 +
 tools/perf/util/evlist.c       |   10 ++-
 tools/perf/util/evlist.h       |    1 +
 tools/perf/util/evsel.c        |   25 +++++++
 tools/perf/util/evsel.h        |   23 ++++++
 tools/perf/util/header.c       |  152 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/header.h       |    2 +
 tools/perf/util/hist.c         |  133 ++++++++++++++++++++++++++++++++---
 tools/perf/util/hist.h         |    1 +
 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         |    1 +
 tools/perf/util/symbol.c       |    4 ++
 tools/perf/util/symbol.h       |    3 +-
 20 files changed, 620 insertions(+), 38 deletions(-)

-- 
1.7.9.2

[PATCH 01/12] perf tools: Keep group information

[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>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evlist.c       |   10 ++++++++--
 tools/perf/util/evlist.h       |    1 +
 tools/perf/util/evsel.h        |   10 ++++++++++
 tools/perf/util/parse-events.c |    1 +
 tools/perf/util/parse-events.h |    1 +
 tools/perf/util/parse-events.y |   10 ++++++++++
 6 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a41dc4a5c2de..561bdfbf8d4f 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -111,20 +111,26 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 void __perf_evlist__set_leader(struct list_head *list)
 {
 	struct perf_evsel *evsel, *leader;
+	int count = 0;
 
 	leader = list_entry(list->next, struct perf_evsel, node);
 	leader->leader = NULL;
 
 	list_for_each_entry(evsel, list, node) {
-		if (evsel != leader)
+		if (evsel != leader) {
 			evsel->leader = leader;
+			evsel->group_idx = count++;
+		}
 	}
+	leader->nr_members = count;
 }
 
 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 6f94d6dea00f..81b6a9740024 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -74,6 +74,10 @@ struct perf_evsel {
 	int			exclude_GH;
 	struct perf_evsel	*leader;
 	char			*group_name;
+	union {
+		int		nr_members;
+		int		group_idx;
+	};
 };
 
 struct cpu_map;
@@ -225,4 +229,10 @@ static inline struct perf_evsel *perf_evsel__next(struct perf_evsel *evsel)
 {
 	return list_entry(evsel->node.next, struct perf_evsel, node);
 }
+
+/* Treat a non-group event as a leader */
+static inline bool perf_evsel__is_group_leader(struct perf_evsel *evsel)
+{
+	return evsel->leader == NULL;
+}
 #endif /* __PERF_EVSEL_H */
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c0b785b50849..8e15c374d600 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -819,6 +819,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 ac9a6aacf2f5..824784b03410 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.9.2

[PATCH 02/12] perf header: Add HEADER_GROUP_DESC feature

[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    |  152 +++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/header.h    |    2 +
 3 files changed, 157 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 5783c3225116..0e66fc3c9972 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -587,6 +587,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..f37a4ecc2ef6 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 > 0) {
+			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 > 0) {
+			fprintf(fp, "# group: %s{%s", evsel->group_name ?: "",
+				perf_evsel__name(evsel));
+
+			nr = evsel->nr_members;
+		} 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,97 @@ 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;
+
+	ret = read(fd, &nr_groups, sizeof(nr_groups));
+	if (ret != 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;
+
+		ret = read(fd, &desc[i].leader_idx, sizeof(u32));
+		if (ret != sizeof(u32))
+			goto out_free;
+
+		ret = read(fd, &desc[i].nr_members, sizeof(u32));
+		if (ret != 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 = NULL;
+			/* {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;
+
+			BUG_ON(i >= nr_groups);
+			BUG_ON(nr > 0);
+
+			leader = evsel;
+			nr = evsel->nr_members;
+			i++;
+		} else if (nr) {
+			/* This is a group member */
+			evsel->leader = leader;
+			/* group_idx starts from 0 */
+			evsel->group_idx = leader->nr_members - nr;
+			nr--;
+		}
+	}
+
+	BUG_ON(i != nr_groups);
+	BUG_ON(nr != 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 +2137,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.9.2

[PATCH 03/12] perf hists: Collapse group hist_entries to a leader

[Namhyung Kim]

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

To support viewing an event group together, collapse all of members in
the group to the leader's tree.  The entries in the leaders' tree will
have group_stats to store those information.

This patch introduced an additional field 'event_group' in symbol_conf
to distinguish whether event grouping is enabled or not.

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/util/evsel.h  |    5 +++
 tools/perf/util/hist.c   |  108 ++++++++++++++++++++++++++++++++++++++++++----
 tools/perf/util/sort.h   |    1 +
 tools/perf/util/symbol.c |    4 ++
 tools/perf/util/symbol.h |    3 +-
 5 files changed, 112 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 81b6a9740024..43341296e50b 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -235,4 +235,9 @@ static inline bool perf_evsel__is_group_leader(struct perf_evsel *evsel)
 {
 	return evsel->leader == NULL;
 }
+
+static inline struct perf_evsel *hists_2_evsel(struct hists *hists)
+{
+	return container_of(hists, struct perf_evsel, hists);
+}
 #endif /* __PERF_EVSEL_H */
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 277947a669b2..95415716c708 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,
@@ -167,6 +168,32 @@ static void he_stat__add_stat(struct he_stat *dest, struct he_stat *src)
 	dest->nr_events		+= src->nr_events;
 }
 
+static void hist_entry__add_group_stat(struct hist_entry *he_dest,
+				       struct he_stat *src,
+				       struct perf_evsel *evsel)
+{
+	struct perf_evsel *leader = evsel->leader;
+
+	if (perf_evsel__is_group_leader(evsel))
+		leader = evsel;
+
+	if (leader->nr_members && !he_dest->group_stats) {
+		/*
+		 * A group whose nr_members equals to 0 is a leader-only group.
+		 * So no need to allocate group_stats.
+		 */
+		he_dest->group_stats = calloc(leader->nr_members,
+					      sizeof(struct he_stat));
+		if (!he_dest->group_stats)
+			return;
+	}
+
+	if (perf_evsel__is_group_leader(evsel))
+		he_stat__add_stat(&he_dest->stat, src);
+	else
+		he_stat__add_stat(&he_dest->group_stats[evsel->group_idx], src);
+}
+
 static void hist_entry__decay(struct hist_entry *he)
 {
 	he->stat.period = (he->stat.period * 7) / 8;
@@ -417,13 +444,14 @@ void hist_entry__free(struct hist_entry *he)
  * collapse the histogram
  */
 
-static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
+static bool hists__collapse_insert_entry(struct hists *hists,
 					 struct rb_root *root,
 					 struct hist_entry *he)
 {
 	struct rb_node **p = &root->rb_node;
 	struct rb_node *parent = NULL;
 	struct hist_entry *iter;
+	struct perf_evsel *evsel = hists_2_evsel(hists);
 	int64_t cmp;
 
 	while (*p != NULL) {
@@ -433,7 +461,10 @@ static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 		cmp = hist_entry__collapse(iter, he);
 
 		if (!cmp) {
-			he_stat__add_stat(&iter->stat, &he->stat);
+			if (symbol_conf.event_group)
+				hist_entry__add_group_stat(iter, &he->stat, evsel);
+			else
+				he_stat__add_stat(&iter->stat, &he->stat);
 
 			if (symbol_conf.use_callchain) {
 				callchain_cursor_reset(&callchain_cursor);
@@ -451,6 +482,19 @@ static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 			p = &(*p)->rb_right;
 	}
 
+	if (symbol_conf.event_group) {
+		/*
+		 * 'he' is not found in the leader's tree.
+		 * Insert it to the tree and setup stats properly.
+		 */
+		hist_entry__add_group_stat(he, &he->stat, evsel);
+
+		if (!perf_evsel__is_group_leader(evsel)) {
+			he->hists = &evsel->leader->hists;
+			memset(&he->stat, 0, sizeof(he->stat));
+		}
+	}
+
 	rb_link_node(&he->rb_node_in, parent, p);
 	rb_insert_color(&he->rb_node_in, root);
 	return true;
@@ -481,6 +525,7 @@ static void hists__apply_filters(struct hists *hists, struct hist_entry *he)
 static void __hists__collapse_resort(struct hists *hists, bool threaded)
 {
 	struct rb_root *root;
+	struct rb_root *dest;
 	struct rb_node *next;
 	struct hist_entry *n;
 
@@ -488,14 +533,26 @@ static void __hists__collapse_resort(struct hists *hists, bool threaded)
 		return;
 
 	root = hists__get_rotate_entries_in(hists);
+	dest = &hists->entries_collapsed;
 	next = rb_first(root);
 
+	if (symbol_conf.event_group) {
+		/*
+		 * Collapse hist entries to the leader's tree.
+		 * If evsel->leader == NULL, it's the leader.
+		 */
+		struct perf_evsel *leader = hists_2_evsel(hists)->leader;
+
+		if (leader)
+			dest = &leader->hists.entries_collapsed;
+	}
+
 	while (next) {
 		n = rb_entry(next, struct hist_entry, rb_node_in);
 		next = rb_next(&n->rb_node_in);
 
 		rb_erase(&n->rb_node_in, root);
-		if (hists__collapse_insert_entry(hists, &hists->entries_collapsed, n)) {
+		if (hists__collapse_insert_entry(hists, dest, n)) {
 			/*
 			 * If it wasn't combined with one of the entries already
 			 * collapsed, we need to apply the filters that may have
@@ -520,13 +577,38 @@ void hists__collapse_resort_threaded(struct hists *hists)
  * reverse the map, sort on period.
  */
 
-static void __hists__insert_output_entry(struct rb_root *entries,
+static int __hists__output_cmp(struct hist_entry *left,
+			       struct hist_entry *right,
+			       int nr_group_stats)
+{
+	if (left->stat.period > right->stat.period)
+		return 1;
+	if (left->stat.period < right->stat.period)
+		return -1;
+
+	if (symbol_conf.event_group) {
+		int i;
+
+		for (i = 0; i < nr_group_stats; i++) {
+			if (left->group_stats[i].period >
+			    right->group_stats[i].period)
+				return 1;
+			if (left->group_stats[i].period <
+			    right->group_stats[i].period)
+				return -1;
+		}
+	}
+	return 0;
+}
+
+static void __hists__insert_output_entry(struct hists *hists,
 					 struct hist_entry *he,
 					 u64 min_callchain_hits)
 {
-	struct rb_node **p = &entries->rb_node;
+	struct rb_node **p = &hists->entries.rb_node;
 	struct rb_node *parent = NULL;
 	struct hist_entry *iter;
+	struct perf_evsel *evsel = hists_2_evsel(hists);
 
 	if (symbol_conf.use_callchain)
 		callchain_param.sort(&he->sorted_chain, he->callchain,
@@ -536,14 +618,14 @@ 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 (__hists__output_cmp(he, iter, evsel->nr_members) > 0)
 			p = &(*p)->rb_left;
 		else
 			p = &(*p)->rb_right;
 	}
 
 	rb_link_node(&he->rb_node, parent, p);
-	rb_insert_color(&he->rb_node, entries);
+	rb_insert_color(&he->rb_node, &hists->entries);
 }
 
 static void __hists__output_resort(struct hists *hists, bool threaded)
@@ -553,6 +635,16 @@ static void __hists__output_resort(struct hists *hists, bool threaded)
 	struct hist_entry *n;
 	u64 min_callchain_hits;
 
+	if (symbol_conf.event_group) {
+		struct perf_evsel *evsel = hists_2_evsel(hists);
+
+		/*
+		 * We've collapsed all member entries to the leader.
+		 */
+		if (!perf_evsel__is_group_leader(evsel))
+			return;
+	}
+
 	min_callchain_hits = hists->stats.total_period * (callchain_param.min_percent / 100);
 
 	if (sort__need_collapse || threaded)
@@ -571,7 +663,7 @@ static void __hists__output_resort(struct hists *hists, bool threaded)
 		n = rb_entry(next, struct hist_entry, rb_node_in);
 		next = rb_next(&n->rb_node_in);
 
-		__hists__insert_output_entry(&hists->entries, n, min_callchain_hits);
+		__hists__insert_output_entry(hists, n, min_callchain_hits);
 		hists__inc_nr_entries(hists, n);
 	}
 }
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 13761d83a5a0..06fab96702f5 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -78,6 +78,7 @@ struct hist_entry {
 	struct rb_node		rb_node_in;
 	struct rb_node		rb_node;
 	struct he_stat		stat;
+	struct he_stat		*group_stats;
 	struct map_symbol	ms;
 	struct thread		*thread;
 	u64			ip;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 624c65e6ab98..3844a1cd9f95 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -11,6 +11,7 @@
 #include <inttypes.h>
 #include "build-id.h"
 #include "util.h"
+#include "sort.h"
 #include "debug.h"
 #include "symbol.h"
 #include "strlist.h"
@@ -1649,6 +1650,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 863b05bea5ff..c6d6355c1d49 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.9.2

[PATCH 04/12] perf hists: Maintain total periods of group members in the leader

[Namhyung Kim]

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

Like group_stats in hist_entry, total periods information also need to
be known to 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/util/hist.c |   25 +++++++++++++++++++++++++
 tools/perf/util/hist.h |    1 +
 2 files changed, 26 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 95415716c708..48488057c3da 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -194,6 +194,28 @@ static void hist_entry__add_group_stat(struct hist_entry *he_dest,
 		he_stat__add_stat(&he_dest->group_stats[evsel->group_idx], src);
 }
 
+static void hists__add_group_stat(struct hists *hists)
+{
+	struct perf_evsel *evsel = hists_2_evsel(hists);
+	struct perf_evsel *leader = evsel->leader;
+	struct hists *leader_hists;
+
+	if (perf_evsel__is_group_leader(evsel))
+		return;
+
+	leader_hists = &leader->hists;
+
+	if (!leader_hists->group_stats) {
+		leader_hists->group_stats = calloc(leader->nr_members,
+						   sizeof(struct events_stats));
+		if (!leader_hists->group_stats)
+			return;
+	}
+
+	memcpy(&leader_hists->group_stats[evsel->group_idx],
+	       &hists->stats, sizeof(struct events_stats));
+}
+
 static void hist_entry__decay(struct hist_entry *he)
 {
 	he->stat.period = (he->stat.period * 7) / 8;
@@ -561,6 +583,9 @@ static void __hists__collapse_resort(struct hists *hists, bool threaded)
 			hists__apply_filters(hists, n);
 		}
 	}
+
+	if (symbol_conf.event_group)
+		hists__add_group_stat(hists);
 }
 
 void hists__collapse_resort(struct hists *hists)
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index a4f45dd04697..99ac4d34c917 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -67,6 +67,7 @@ struct hists {
 	const char		*symbol_filter_str;
 	pthread_mutex_t		lock;
 	struct events_stats	stats;
+	struct events_stats	*group_stats;
 	u64			event_stream;
 	u16			col_len[HISTC_NR_COLS];
 };
-- 
1.7.9.2

[PATCH 05/12] perf report: Make another loop for output resorting

[Namhyung Kim]

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

Now the event grouping viewing requires collapsing all members in a
group to the leader.  Thus hists__output_resort should be called after
collapsing all entries in evlist.

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-report.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index fc251005dd3d..1dc9b4bd5da8 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -416,6 +416,11 @@ static int __cmd_report(struct perf_report *rep)
 			hists->symbol_filter_str = rep->symbol_filter_str;
 
 		hists__collapse_resort(hists);
+	}
+
+	list_for_each_entry(pos, &session->evlist->entries, node) {
+		struct hists *hists = &pos->hists;
+
 		hists__output_resort(hists);
 		nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE];
 	}
-- 
1.7.9.2

[PATCH 06/12] perf ui/hist: Add support for event group view

[Namhyung Kim]

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

Show group members' overhead also when showing the leader's if event
group is enabled.  At this time, only implemented overhead part in
order to ease review and other parts can be added later once this
patch settled down.

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/ui/hist.c       |   66 ++++++++++++++++++++++++++++++++++++++++----
 tools/perf/ui/stdio/hist.c |    2 ++
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 4f5f4756faac..c406c94cbe53 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -3,34 +3,90 @@
 #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");
+	int len = 8;
+
+	if (symbol_conf.event_group) {
+		struct perf_evsel *evsel = hpp->ptr;
+
+		BUG_ON(!perf_evsel__is_group_leader(evsel));
+
+		len += evsel->nr_members * 8;
+	}
+	return scnprintf(hpp->buf, hpp->size, "%*s", len, "Overhead");
 }
 
-static int hpp__width_overhead(struct perf_hpp *hpp __maybe_unused)
+static int hpp__width_overhead(struct perf_hpp *hpp)
 {
-	return 8;
+	int len = 8;
+
+	if (symbol_conf.event_group) {
+		struct perf_evsel *evsel = hpp->ptr;
+
+		len += evsel->nr_members * 8;
+	}
+	return len;
 }
 
 static int hpp__color_overhead(struct perf_hpp *hpp, struct hist_entry *he)
 {
+	int ret;
 	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);
+	ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent);
+
+	if (symbol_conf.event_group) {
+		int i;
+		struct perf_evsel *evsel = hists_2_evsel(hists);
+
+		for (i = 0; i < evsel->nr_members; i++) {
+			u64 period = he->group_stats[i].period;
+			u64 total = hists->group_stats[i].total_period;
+
+			percent = 100.0 * period / total;
+			ret += percent_color_snprintf(hpp->buf + ret,
+						      hpp->size - ret,
+						      " %6.2f%%", percent);
+		}
+
+	}
+	return ret;
 }
 
 static int hpp__entry_overhead(struct perf_hpp *hpp, struct hist_entry *he)
 {
+	int ret;
 	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);
+	ret = scnprintf(hpp->buf, hpp->size, fmt, percent);
+
+	if (symbol_conf.event_group) {
+		int i;
+		struct perf_evsel *evsel = hists_2_evsel(hists);
+
+		for (i = 0; i < evsel->nr_members; i++) {
+			u64 period = he->group_stats[i].period;
+			u64 total = hists->group_stats[i].total_period;
+
+			if (symbol_conf.field_sep) {
+				ret += scnprintf(hpp->buf + ret,
+						 hpp->size - ret, " ");
+			}
+			percent = 100.0 * period / total;
+			ret += scnprintf(hpp->buf + ret, hpp->size - ret,
+					 fmt, percent);
+		}
+
+	}
+	return ret;
 }
 
 static int hpp__header_overhead_sys(struct perf_hpp *hpp)
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index f0ee204f99bb..ca5ee86ab437 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_2_evsel(hists),
 	};
 	bool first = true;
 
-- 
1.7.9.2

[PATCH 07/12] perf ui/browser: Add support for event group view

[Namhyung Kim]

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

Show group members' overhead also when showing the leader's if event
group is enabled.  At this time, only implemented overhead part in
order to ease review and other parts can be added later once this
patch settled down.

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/ui/browsers/hists.c |   29 ++++++++++++++++++++++++++++-
 tools/perf/ui/hist.c           |    5 ++++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index ccc4bd161420..ffd41cbe07df 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -567,6 +567,34 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
 	return row - first_row;
 }
 
+static int hist_browser__hpp_color_overhead(struct perf_hpp *hpp,
+					    struct hist_entry *he)
+{
+	int ret;
+	struct hists *hists = he->hists;
+	double percent = 100.0 * he->stat.period / hists->stats.total_period;
+
+	/* the leader determines color */
+	*(double *) hpp->ptr = percent;
+
+	ret = scnprintf(hpp->buf, hpp->size, "%6.2f%%", percent);
+
+	if (symbol_conf.event_group) {
+		int i;
+		struct perf_evsel *evsel = hists_2_evsel(hists);
+
+		for (i = 0; i < evsel->nr_members; i++) {
+			u64 period = he->group_stats[i].period;
+			u64 total = hists->group_stats[i].total_period;
+
+			percent = 100.0 * period / total;
+			ret += scnprintf(hpp->buf + ret, hpp->size - ret,
+					 " %6.2f%%", percent);
+		}
+	}
+	return ret;
+}
+
 #define HPP__COLOR_FN(_name, _field)					\
 static int hist_browser__hpp_color_ ## _name(struct perf_hpp *hpp,	\
 					     struct hist_entry *he)	\
@@ -577,7 +605,6 @@ static int hist_browser__hpp_color_ ## _name(struct perf_hpp *hpp,	\
 	return scnprintf(hpp->buf, hpp->size, "%6.2f%%", percent);	\
 }
 
-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)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index c406c94cbe53..51934a8d9e79 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -562,6 +562,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_2_evsel(hists),
+	};
 
 	for (i = 0; i < PERF_HPP__MAX_INDEX; i++) {
 		if (!perf_hpp__format[i].cond)
@@ -569,7 +572,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.9.2

[PATCH 08/12] perf ui/gtk: Add support for event group view

[Namhyung Kim]

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

Show group members' overhead also when showing the leader's if event
group is enabled.  At this time, only implemented overhead part in
order to ease review and other parts can be added later once this
patch settled down.

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/ui/gtk/browser.c |   60 +++++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/tools/perf/ui/gtk/browser.c b/tools/perf/ui/gtk/browser.c
index 253b6219a39e..293eb4ac5e9c 100644
--- a/tools/perf/ui/gtk/browser.c
+++ b/tools/perf/ui/gtk/browser.c
@@ -45,26 +45,57 @@ static const char *perf_gtk__get_percent_color(double percent)
 	return NULL;
 }
 
+static int perf_gtk__percent_color_snprintf(char *buf, size_t size,
+					    u64 period, u64 total_period)
+{
+	int ret = 0;
+	const char *markup;
+	double percent = 100.0 * period / total_period;
+
+	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;
+}
+
+static int perf_gtk__hpp_color_overhead(struct perf_hpp *hpp,
+					struct hist_entry *he)
+{
+	int ret;
+	struct hists *hists = he->hists;
+
+	ret = perf_gtk__percent_color_snprintf(hpp->buf, hpp->size,
+				he->stat.period, hists->stats.total_period);
+
+	if (symbol_conf.event_group) {
+		int i;
+		struct perf_evsel *evsel = hists_2_evsel(hists);
+
+		for (i = 0; i < evsel->nr_members; i++) {
+			ret += scnprintf(hpp->buf + ret, hpp->size - ret, " ");
+			ret += perf_gtk__percent_color_snprintf(hpp->buf + ret,
+					hpp->size - ret,
+					he->group_stats[i].period,
+					hists->group_stats[i].total_period);
+		}
+	}
+	return ret;
+}
+
 #define HPP__COLOR_FN(_name, _field)						\
 static int perf_gtk__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;	\
-	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>"); 	\
-										\
-	return ret;								\
+	return perf_gtk__percent_color_snprintf(hpp->buf, hpp->size, 		\
+			he->stat._field, he->hists->stats.total_period);	\
 }
 
-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)
@@ -103,6 +134,7 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists)
 	struct perf_hpp hpp = {
 		.buf		= s,
 		.size		= sizeof(s),
+		.ptr		= hists_2_evsel(hists),
 	};
 
 	nr_cols = 0;
-- 
1.7.9.2

[PATCH 09/12] perf report: Bypass non-leader events when event group is enabled

[Namhyung Kim]

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 1dc9b4bd5da8..94011af2a747 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 ffd41cbe07df..894d9bbc81c8 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1631,8 +1631,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)
 {
@@ -1643,7 +1654,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,
@@ -1659,20 +1671,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 293eb4ac5e9c..10cb81849566 100644
--- a/tools/perf/ui/gtk/browser.c
+++ b/tools/perf/ui/gtk/browser.c
@@ -304,6 +304,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.9.2

[PATCH 10/12] perf report: Show group description when event group is enabled

[Namhyung Kim]

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    |   18 ++++++++++++++++++
 tools/perf/ui/browsers/hists.c |   31 +++++++++++++++++++++++++++++++
 tools/perf/ui/gtk/browser.c    |   14 +++++++++++---
 tools/perf/util/evsel.c        |   25 +++++++++++++++++++++++++
 tools/perf/util/evsel.h        |    8 ++++++++
 5 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 94011af2a747..be4941afa658 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -299,6 +299,24 @@ 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_2_evsel(self);
+	char buf[512];
+	size_t size = sizeof(buf);
+
+	if (symbol_conf.event_group && evsel->nr_members) {
+		int i;
+		struct events_stats *stats;
+
+		perf_evsel__group_desc(evsel, buf, size);
+		evname = buf;
+
+		for (i = 0; i < evsel->nr_members; i++) {
+			stats = &self->group_stats[i];
+
+			nr_samples += stats->nr_events[PERF_RECORD_SAMPLE];
+			nr_events += 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 894d9bbc81c8..893c07f172f5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1125,6 +1125,24 @@ 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_2_evsel(hists);
+	char buf[512];
+	size_t buflen = sizeof(buf);
+
+	if (symbol_conf.event_group && evsel->nr_members) {
+		int i;
+		struct events_stats *stats;
+
+		perf_evsel__group_desc(evsel, buf, buflen);
+		ev_name = buf;
+
+		for (i = 0; i < evsel->nr_members; i++) {
+			stats = &hists->group_stats[i];
+
+			nr_samples += stats->nr_events[PERF_RECORD_SAMPLE];
+			nr_events += stats->total_period;
+		}
+	}
 
 	nr_samples = convert_unit(nr_samples, &unit);
 	printed = scnprintf(bf, size,
@@ -1521,6 +1539,19 @@ 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) {
+		int i;
+		struct events_stats *stats;
+
+		ev_name = perf_evsel__group_name(evsel);
+
+		for (i = 0; i < evsel->nr_members; i++) {
+			stats = &evsel->hists.group_stats[i];
+
+			nr_events += 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 10cb81849566..a8670c2e748c 100644
--- a/tools/perf/ui/gtk/browser.c
+++ b/tools/perf/ui/gtk/browser.c
@@ -303,10 +303,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) {
+				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 618d41140abd..c5ecd6b7d92b 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -404,6 +404,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;
+}
+
 void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
 			struct perf_evsel *first)
 {
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 43341296e50b..ae32b1c9ed0a 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -114,6 +114,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);
@@ -240,4 +242,10 @@ static inline struct perf_evsel *hists_2_evsel(struct hists *hists)
 {
 	return container_of(hists, struct perf_evsel, hists);
 }
+
+#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.9.2

[PATCH 11/12] perf report: Add --group option

[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/builtin-report.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index be4941afa658..1a1a650fe952 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -665,6 +665,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.9.2

[PATCH 12/12] perf report: Add report.group config option

[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 1a1a650fe952..b7419cd3889a 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,
@@ -674,6 +685,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.9.2

Re: [PATCH 01/12] perf tools: Keep group information

[Arnaldo Carvalho de Melo]

Em Sat, Nov 10, 2012 at 01:43:23AM +0900, Namhyung Kim escreveu:
> 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.

We don't need to add a group_idx, we can get it using evsel->idx -
evsel->leader->idx, interdiff of this patch to what I think is
equivalent:

diff -u b/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
--- b/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -111,18 +111,16 @@
 void __perf_evlist__set_leader(struct list_head *list)
 {
 	struct perf_evsel *evsel, *leader;
-	int count = 0;
 
 	leader = list_entry(list->next, struct perf_evsel, node);
+	evsel = list_entry(list->prev, struct perf_evsel, node);
 	leader->leader = NULL;
+	leader->nr_members = evsel->idx - leader->idx + 1;
 
 	list_for_each_entry(evsel, list, node) {
-		if (evsel != leader) {
+		if (evsel != leader)
 			evsel->leader = leader;
-			evsel->group_idx = count++;
-		}
 	}
-	leader->nr_members = count;
 }
 
 void perf_evlist__set_leader(struct perf_evlist *evlist)
diff -u b/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
--- b/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -72,12 +72,9 @@
 	bool 			needs_swap;
 	/* parse modifier helper */
 	int			exclude_GH;
+	int			nr_members;
 	struct perf_evsel	*leader;
 	char			*group_name;
-	union {
-		int		nr_members;
-		int		group_idx;
-	};
 };
 
 struct cpu_map;
@@ -237,2 +234,11 @@
 }
+
+static inline int perf_evsel__group_idx(struct perf_evsel *evsel)
+{
+	if (perf_evsel__is_group_leader(evsel)
+		return 0;
+
+	return evsel->idx - evsel->leader->idx;
+}
+
 #endif /* __PERF_EVSEL_H */

Re: [PATCH 02/12] perf header: Add HEADER_GROUP_DESC feature

[Arnaldo Carvalho de Melo]

Em Sat, Nov 10, 2012 at 01:43:24AM +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>

<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;

why initialize ret if the first thing you do with it is...

> +	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;
> +
> +	ret = read(fd, &nr_groups, sizeof(nr_groups));

assign another value?

We need to use readn instead of read, and not use ret here, just do
	if (read(fd, &nr_groups, sizeof(nr_groups)) != sizeof(nr_groups))

> +	if (ret != 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;
> +
> +		ret = read(fd, &desc[i].leader_idx, sizeof(u32));
> +		if (ret != sizeof(u32))
> +			goto out_free;

ditto

> +
> +		ret = read(fd, &desc[i].nr_members, sizeof(u32));
> +		if (ret != sizeof(u32))
> +			goto out_free;

ditto

> +
> +		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 = NULL;

Humm, isn't it better to set evsel->leader to evsel, that way all
members in a group have its evsel->leader pointing to the leader and for
things like the group idx we can do just:

		evsel->idx - evsel->leader->idx

avoiding the need to special case the leader, will do that.

> +			/* {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;
> +
> +			BUG_ON(i >= nr_groups);
> +			BUG_ON(nr > 0);

BUG_ON here is way too extreme, we should just bail out, testing and if
the problem is found, goto out_free

> +			leader = evsel;
> +			nr = evsel->nr_members;
> +			i++;
> +		} else if (nr) {

Humm, do we really need to test against nr? Or even use nr at all?

> +			/* This is a group member */
> +			evsel->leader = leader;
> +			/* group_idx starts from 0 */
> +			evsel->group_idx = leader->nr_members - nr;
> +			nr--;
> +		}
> +	}
> +
> +	BUG_ON(i != nr_groups);
> +	BUG_ON(nr != 0);

Too extreme, goto out_free.

And just before out_free we need to set ret = 0, otherwise we will
return...

> +out_free:
> +	while ((int) --i >= 0)
> +		free(desc[i].name);
> +	free(desc);

here, whatever value was in ret, in this patch it will be, if all goes
well, sizeof(u32) :-)

> +	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 +2137,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,
>  };

Here we're adding holes to the struct:

> @@ -79,6 +80,7 @@ struct perf_session_env {
>  	char			*numa_nodes;
>  	int			nr_pmu_mappings;

4 byte hole

>  	char			*pmu_mappings;

4 byte hole

> +	int			nr_groups;

I'm working on a patch implementing my suggestions.

>  };
>  
>  struct perf_header {
> -- 
> 1.7.9.2

Re: [PATCH 01/12] perf tools: Keep group information

[Namhyung Kim]

On Mon, 12 Nov 2012 14:08:33 -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Nov 10, 2012 at 01:43:23AM +0900, Namhyung Kim escreveu:
>> 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.
>
> We don't need to add a group_idx, we can get it using evsel->idx -
> evsel->leader->idx, interdiff of this patch to what I think is
> equivalent:

Looks good to me.  Just a comment below..

>
> diff -u b/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> --- b/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -111,18 +111,16 @@
>  void __perf_evlist__set_leader(struct list_head *list)
>  {
>  	struct perf_evsel *evsel, *leader;
> -	int count = 0;
>  
>  	leader = list_entry(list->next, struct perf_evsel, node);
> +	evsel = list_entry(list->prev, struct perf_evsel, node);
>  	leader->leader = NULL;
> +	leader->nr_members = evsel->idx - leader->idx + 1;

This will lead a off-by-one difference with my patches.  I chose to use
0 nr_members for leader-only group.  It was used for an additional
allocation of group stats but it seems that it's not needed anymore
with the hist_link change.

Thanks,
Namhyung

Re: [PATCH 02/12] perf header: Add HEADER_GROUP_DESC feature

[Namhyung Kim]

On Mon, 12 Nov 2012 14:45:34 -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Nov 10, 2012 at 01:43:24AM +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>
>
> <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;
>
> why initialize ret if the first thing you do with it is...
>
>> +	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;
>> +
>> +	ret = read(fd, &nr_groups, sizeof(nr_groups));
>
> assign another value?
>
> We need to use readn instead of read, and not use ret here, just do
> 	if (read(fd, &nr_groups, sizeof(nr_groups)) != sizeof(nr_groups))

Will do.

>
>> +	if (ret != 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;
>> +
>> +		ret = read(fd, &desc[i].leader_idx, sizeof(u32));
>> +		if (ret != sizeof(u32))
>> +			goto out_free;
>
> ditto
>
>> +
>> +		ret = read(fd, &desc[i].nr_members, sizeof(u32));
>> +		if (ret != sizeof(u32))
>> +			goto out_free;
>
> ditto
>
>> +
>> +		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 = NULL;
>
> Humm, isn't it better to set evsel->leader to evsel, that way all
> members in a group have its evsel->leader pointing to the leader and for
> things like the group idx we can do just:
>
> 		evsel->idx - evsel->leader->idx
>
> avoiding the need to special case the leader, will do that.

I just followed the existing behavior.  I'm open to changing it. :)

>
>> +			/* {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;
>> +
>> +			BUG_ON(i >= nr_groups);
>> +			BUG_ON(nr > 0);
>
> BUG_ON here is way too extreme, we should just bail out, testing and if
> the problem is found, goto out_free

Okay, it was there to help me debugging the code. Will change.

>
>> +			leader = evsel;
>> +			nr = evsel->nr_members;
>> +			i++;
>> +		} else if (nr) {
>
> Humm, do we really need to test against nr? Or even use nr at all?

The group descriptor misses leader-only groups intentionally.  In that
case I wanted to skip those events with nr == 0.

>
>> +			/* This is a group member */
>> +			evsel->leader = leader;
>> +			/* group_idx starts from 0 */
>> +			evsel->group_idx = leader->nr_members - nr;
>> +			nr--;
>> +		}
>> +	}
>> +
>> +	BUG_ON(i != nr_groups);
>> +	BUG_ON(nr != 0);
>
> Too extreme, goto out_free.
>
> And just before out_free we need to set ret = 0, otherwise we will
> return...

Right, will do.

>
>> +out_free:
>> +	while ((int) --i >= 0)
>> +		free(desc[i].name);
>> +	free(desc);
>
> here, whatever value was in ret, in this patch it will be, if all goes
> well, sizeof(u32) :-)
>
>> +	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 +2137,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,
>>  };
>
> Here we're adding holes to the struct:
>
>> @@ -79,6 +80,7 @@ struct perf_session_env {
>>  	char			*numa_nodes;
>>  	int			nr_pmu_mappings;
>
> 4 byte hole
>
>>  	char			*pmu_mappings;
>
> 4 byte hole

Yes, I thought it's a in-memory struct for a session, so a couple of
holes doesn't matter much.  So I just wanted to keep the struct in a
consistent and straight-forward way.

>
>> +	int			nr_groups;
>
> I'm working on a patch implementing my suggestions.

Great!

Thanks,
Namhyung

[PATCH 09/12] perf report: Bypass non-leader events when event group is enabled

[Namhyung Kim]

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 | 39 +++++++++++++++++++++++++++++++++------
 tools/perf/ui/gtk/browser.c    |  4 ++++
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index bacf12a4e0b9..6c73d8625829 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -320,6 +320,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 0f8be070aedd..e699973c0581 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1578,8 +1578,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,
 					   void(*timer)(void *arg), void *arg,
 					   int delay_secs)
 {
@@ -1590,7 +1601,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,
 		},
 	};
@@ -1605,7 +1617,7 @@ 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, timer,
+	return perf_evsel_menu__run(&menu, nr_entries, help, timer,
 				    arg, delay_secs);
 }
 
@@ -1613,15 +1625,30 @@ int perf_evlist__tui_browse_hists(struct perf_evlist *evlist, const char *help,
 				  void(*timer)(void *arg), void *arg,
 				  int delay_secs)
 {
-	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, timer, arg,
 						delay_secs);
 	}
 
-	return __perf_evlist__tui_browse_hists(evlist, help,
+	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,
 					       timer, arg, delay_secs);
 }
diff --git a/tools/perf/ui/gtk/browser.c b/tools/perf/ui/gtk/browser.c
index b2c8b8e69e4d..ae6b941ef3d4 100644
--- a/tools/perf/ui/gtk/browser.c
+++ b/tools/perf/ui/gtk/browser.c
@@ -306,6 +306,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