[PATCHv3 0/3] perf tool: Add new event group management

[PATCHv3 0/3] perf tool: Add new event group management

[Jiri Olsa]

hi,
adding support for creating event groups based on the way they
are specified on the command line.

This patchset adds the '{}' style grammar to express event group,
allowing so far only the 'event modifier' as group modifier.

The sample/leader modifier changes are ready, but I'm getting
some weird numbers in tests, so I'll send it later. Meanwhile,
it'd be nice to have at least this patchset in.. ;)


Attached patches:
  1/3 perf, tool: Add support to parse event group syntax
  2/3 perf, tool: Enable grouping logic for parsed events
  3/3 perf, test: Add automated tests for event group parsing

wbr,
jirka
---
 tools/perf/builtin-record.c         |   13 +--
 tools/perf/builtin-stat.c           |   13 +--
 tools/perf/builtin-test.c           |    8 +-
 tools/perf/builtin-top.c            |   12 +--
 tools/perf/util/evlist.c            |   20 ++---
 tools/perf/util/evlist.h            |    3 +-
 tools/perf/util/evsel.c             |   51 ++++++++----
 tools/perf/util/evsel.h             |   11 +--
 tools/perf/util/parse-events-test.c |  152 +++++++++++++++++++++++++++++++++++
 tools/perf/util/parse-events.c      |   32 +++++++-
 tools/perf/util/parse-events.h      |    5 +-
 tools/perf/util/parse-events.l      |    2 +
 tools/perf/util/parse-events.y      |   93 ++++++++++++++++++---
 tools/perf/util/python.c            |    7 +-
 14 files changed, 341 insertions(+), 81 deletions(-)

[PATCH 1/3] perf, tool: Add support to parse event group syntax

[Jiri Olsa]

Adding scanner/parser bits to parse event groups.

The grammar for group is:
  groups:      groups ',' group | group
  group:       group_name '{' events '}' group_mod
  group_name:  name | empty
  group_mod:   ':' group_mods | empty
  group_mods:  event_mod

Currently it's possible to use standard event modifier as a
modifier for group. It spans over all events in the group
and overrides any event modifier settings.

It's necessary to use quoting ("'\) when specifying group on
command line, since {} characters are interpretted by most of
the shells.

It is now possible to specify groups in event syntax like:

  '{cycles,faults}'
   - anonymous group

  'group1{cycles,faults}
   - group with name 'group1'

  '{cycles,faults}:k
   - anonymous group with event modifier 'k'

  '{cpu-clock,task-clock},{minor-faults,major-faults}'
   - two anonymous groups

The grouping functionality itself is comming shortly.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/parse-events.c |   14 +++++-
 tools/perf/util/parse-events.h |    4 +-
 tools/perf/util/parse-events.l |    2 +
 tools/perf/util/parse-events.y |   93 ++++++++++++++++++++++++++++++++++------
 4 files changed, 97 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 0cc27da..f62cbee 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -547,19 +547,29 @@ int parse_events_add_pmu(struct list_head **list, int *idx,
 			 pmu_event_name(head_config));
 }
 
+int parse_events__modifier_group(struct list_head *list __used,
+				 char *event_mod __used)
+{
+	return 0;
+}
+
+void parse_events__group(char *name __used, struct list_head *list __used)
+{
+}
+
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all)
 {
 	/*
 	 * Called for single event definition. Update the
-	 * 'all event' list, and reinit the 'signle event'
+	 * 'all event' list, and reinit the 'single event'
 	 * list, for next event definition.
 	 */
 	list_splice_tail(list_event, list_all);
 	free(list_event);
 }
 
-int parse_events_modifier(struct list_head *list, char *str)
+int parse_events__modifier_event(struct list_head *list, char *str)
 {
 	struct perf_evsel *evsel;
 	int exclude = 0, exclude_GH = 0;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index ee9c218..c2f2ed9 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -79,7 +79,8 @@ int parse_events__term_str(struct parse_events__term **_term,
 int parse_events__term_clone(struct parse_events__term **new,
 			     struct parse_events__term *term);
 void parse_events__free_terms(struct list_head *terms);
-int parse_events_modifier(struct list_head *list, char *str);
+int parse_events__modifier_event(struct list_head *list, char *str);
+int parse_events__modifier_group(struct list_head *list, char *event_mod);
 int parse_events_add_tracepoint(struct list_head **list, int *idx,
 				char *sys, char *event);
 int parse_events_add_numeric(struct list_head **list, int *idx,
@@ -91,6 +92,7 @@ int parse_events_add_breakpoint(struct list_head **list, int *idx,
 				void *ptr, char *type);
 int parse_events_add_pmu(struct list_head **list, int *idx,
 			 char *pmu , struct list_head *head_config);
+void parse_events__group(char *name, struct list_head *list);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
 void parse_events_error(void *data, void *scanner, char const *msg);
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 488362e..931bb6e 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -151,6 +151,8 @@ r{num_raw_hex}		{ return raw(yyscanner); }
 -			{ return '-'; }
 ,			{ return ','; }
 :			{ return ':'; }
+"{"			{ return '{'; }
+"}"			{ return '}'; }
 =			{ return '='; }
 
 <mem>{
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 9525c45..2141c0c 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -30,7 +30,7 @@ do { \
 %token PE_NAME
 %token PE_MODIFIER_EVENT PE_MODIFIER_BP
 %token PE_NAME_CACHE_TYPE PE_NAME_CACHE_OP_RESULT
-%token PE_PREFIX_MEM PE_PREFIX_RAW
+%token PE_PREFIX_MEM PE_PREFIX_RAW PE_PREFIX_GROUP
 %token PE_ERROR
 %type <num> PE_VALUE
 %type <num> PE_VALUE_SYM
@@ -51,6 +51,11 @@ do { \
 %type <head> event_legacy_numeric
 %type <head> event_legacy_raw
 %type <head> event_def
+%type <head> event
+%type <head> events
+%type <head> group_def
+%type <head> group
+%type <head> groups
 
 %union
 {
@@ -62,33 +67,95 @@ do { \
 %%
 
 start:
-PE_START_EVENTS events
+PE_START_EVENTS start_events
 |
-PE_START_TERMS  terms
+PE_START_TERMS  start_terms
+
+start_events: groups
+{
+	struct parse_events_data__events *data = _data;
+
+	parse_events_update_lists($1, &data->list);
+}
+
+groups:
+groups ',' group
+{
+	struct list_head *list  = $1;
+	struct list_head *group = $3;
+
+	parse_events_update_lists(group, list);
+	$$ = list;
+}
+|
+groups ',' event
+{
+	struct list_head *list  = $1;
+	struct list_head *event = $3;
+
+	parse_events_update_lists(event, list);
+	$$ = list;
+}
+|
+group
+|
+event
+
+group:
+group_def ':' PE_MODIFIER_EVENT
+{
+	struct list_head *list = $1;
+
+	ABORT_ON(parse_events__modifier_group(list, $3));
+	$$ = list;
+}
+|
+group_def
+
+group_def:
+PE_NAME '{' events '}'
+{
+	struct list_head *list = $3;
+
+	parse_events__group($1, list);
+	$$ = list;
+}
+|
+'{' events '}'
+{
+	struct list_head *list = $2;
+
+	parse_events__group(NULL, list);
+	$$ = list;
+}
 
 events:
-events ',' event | event
+events ',' event
+{
+	struct list_head *event = $3;
+	struct list_head *list  = $1;
+
+	parse_events_update_lists(event, list);
+	$$ = list;
+}
+|
+event
 
 event:
 event_def PE_MODIFIER_EVENT
 {
-	struct parse_events_data__events *data = _data;
+	struct list_head *list = $1;
 
 	/*
 	 * Apply modifier on all events added by single event definition
 	 * (there could be more events added for multiple tracepoint
 	 * definitions via '*?'.
 	 */
-	ABORT_ON(parse_events_modifier($1, $2));
-	parse_events_update_lists($1, &data->list);
+	ABORT_ON(parse_events__modifier_event(list, $2));
+	$$ = list;
 }
 |
 event_def
-{
-	struct parse_events_data__events *data = _data;
-
-	parse_events_update_lists($1, &data->list);
-}
 
 event_def: event_pmu |
 	   event_legacy_symbol |
@@ -215,7 +282,7 @@ PE_RAW
 	$$ = list;
 }
 
-terms: event_config
+start_terms: event_config
 {
 	struct parse_events_data__terms *data = _data;
 	data->terms = $1;
-- 
1.7.7.6

[PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Jiri Olsa]

This patch adds a functionality that allows to create event groups
based on the way they are specified on the command line. Adding
functionality to the '{}' group syntax introduced in earlier patch.

The current '--group/-g' option behaviour remains intact.  If you
specify it for record/stat/top command, all the specified events
become members of a single group with the first event as a group
leader.

With the new '{}' group syntax you can create group like:
  # perf record -e '{cycles,faults}' ls

resulting in single event group containing 'cycles' and 'faults'
events, with cycles event as group leader.

All groups are created with regards to threads and cpus. Thus
recording an event group within a 2 threads on server with
4 CPUs will create 8 separate groups.

Examples (first event in brackets is group leader):

  # 1 group (cpu-clock,task-clock)
  perf record --group -e cpu-clock,task-clock ls
  perf record -e '{cpu-clock,task-clock}' ls

  # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
  perf record -e '{cpu-clock,task-clock},{minor-faults,major-faults}' ls

  # 1 group (cpu-clock,task-clock,minor-faults,major-faults)
  perf record --group -e cpu-clock,task-clock -e minor-faults,major-faults ls
  perf record -e '{cpu-clock,task-clock,minor-faults,major-faults}' ls

  # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
  perf record -e '{cpu-clock,task-clock} -e '{minor-faults,major-faults}' \
   -e instructions ls

  # 1 group (cpu-clock,task-clock,minor-faults,major-faults,instructions)
  perf record --group -e cpu-clock,task-clock \
   -e minor-faults,major-faults -e instructions ls
  perf record -e '{cpu-clock,task-clock,minor-faults,major-faults,instructions}' ls

It's possible to use standard event modifier for a group, which spans
over all events in the group and overrides any event modifier settings,
for example:

  # perf record -r '{faults:k,cache-references}:u'

resulting in ':u' modifier being used for both 'faults' and 'cache-references'
events, regardless of their modifier setup (':k' for faults event).

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/builtin-record.c    |   13 ++++------
 tools/perf/builtin-stat.c      |   13 ++++------
 tools/perf/builtin-test.c      |    8 +++---
 tools/perf/builtin-top.c       |   12 +++------
 tools/perf/util/evlist.c       |   20 +++++++--------
 tools/perf/util/evlist.h       |    3 +-
 tools/perf/util/evsel.c        |   51 ++++++++++++++++++++++++++-------------
 tools/perf/util/evsel.h        |   11 ++++----
 tools/perf/util/parse-events.c |   26 +++++++++++++++++---
 tools/perf/util/parse-events.h |    1 +
 tools/perf/util/python.c       |    7 ++++-
 11 files changed, 96 insertions(+), 69 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f5a6452..94d6b12 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -176,18 +176,18 @@ static bool perf_evlist__equal(struct perf_evlist *evlist,
 
 static void perf_record__open(struct perf_record *rec)
 {
-	struct perf_evsel *pos, *first;
+	struct perf_evsel *pos;
 	struct perf_evlist *evlist = rec->evlist;
 	struct perf_session *session = rec->session;
 	struct perf_record_opts *opts = &rec->opts;
 
-	first = list_entry(evlist->entries.next, struct perf_evsel, node);
-
 	perf_evlist__config_attrs(evlist, opts);
 
+	if (opts->group)
+		perf_evlist__group(evlist);
+
 	list_for_each_entry(pos, &evlist->entries, node) {
 		struct perf_event_attr *attr = &pos->attr;
-		struct xyarray *group_fd = NULL;
 		/*
 		 * Check if parse_single_tracepoint_event has already asked for
 		 * PERF_SAMPLE_TIME.
@@ -202,16 +202,13 @@ static void perf_record__open(struct perf_record *rec)
 		 */
 		bool time_needed = attr->sample_type & PERF_SAMPLE_TIME;
 
-		if (opts->group && pos != first)
-			group_fd = first->fd;
 fallback_missing_features:
 		if (opts->exclude_guest_missing)
 			attr->exclude_guest = attr->exclude_host = 0;
 retry_sample_id:
 		attr->sample_id_all = opts->sample_id_all_missing ? 0 : 1;
 try_again:
-		if (perf_evsel__open(pos, evlist->cpus, evlist->threads,
-				     opts->group, group_fd) < 0) {
+		if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) {
 			int err = errno;
 
 			if (err == EPERM || err == EACCES) {
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 861f0ae..23908a8 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -281,13 +281,9 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 				    struct perf_evsel *first)
 {
 	struct perf_event_attr *attr = &evsel->attr;
-	struct xyarray *group_fd = NULL;
 	bool exclude_guest_missing = false;
 	int ret;
 
-	if (group && evsel != first)
-		group_fd = first->fd;
-
 	if (scale)
 		attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED |
 				    PERF_FORMAT_TOTAL_TIME_RUNNING;
@@ -299,8 +295,7 @@ retry:
 		evsel->attr.exclude_guest = evsel->attr.exclude_host = 0;
 
 	if (perf_target__has_cpu(&target)) {
-		ret = perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
-					       group, group_fd);
+		ret = perf_evsel__open_per_cpu(evsel, evsel_list->cpus);
 		if (ret)
 			goto check_ret;
 		return 0;
@@ -311,8 +306,7 @@ retry:
 		attr->enable_on_exec = 1;
 	}
 
-	ret = perf_evsel__open_per_thread(evsel, evsel_list->threads,
-					  group, group_fd);
+	ret = perf_evsel__open_per_thread(evsel, evsel_list->threads);
 	if (!ret)
 		return 0;
 	/* fall through */
@@ -483,6 +477,9 @@ static int run_perf_stat(int argc __used, const char **argv)
 		close(child_ready_pipe[0]);
 	}
 
+	if (group)
+		perf_evlist__group(evsel_list);
+
 	first = list_entry(evsel_list->entries.next, struct perf_evsel, node);
 
 	list_for_each_entry(counter, &evsel_list->entries, node) {
diff --git a/tools/perf/builtin-test.c b/tools/perf/builtin-test.c
index 5ce3030..03568b8 100644
--- a/tools/perf/builtin-test.c
+++ b/tools/perf/builtin-test.c
@@ -294,7 +294,7 @@ static int test__open_syscall_event(void)
 		goto out_thread_map_delete;
 	}
 
-	if (perf_evsel__open_per_thread(evsel, threads, false, NULL) < 0) {
+	if (perf_evsel__open_per_thread(evsel, threads) < 0) {
 		pr_debug("failed to open counter: %s, "
 			 "tweak /proc/sys/kernel/perf_event_paranoid?\n",
 			 strerror(errno));
@@ -369,7 +369,7 @@ static int test__open_syscall_event_on_all_cpus(void)
 		goto out_thread_map_delete;
 	}
 
-	if (perf_evsel__open(evsel, cpus, threads, false, NULL) < 0) {
+	if (perf_evsel__open(evsel, cpus, threads) < 0) {
 		pr_debug("failed to open counter: %s, "
 			 "tweak /proc/sys/kernel/perf_event_paranoid?\n",
 			 strerror(errno));
@@ -534,7 +534,7 @@ static int test__basic_mmap(void)
 
 		perf_evlist__add(evlist, evsels[i]);
 
-		if (perf_evsel__open(evsels[i], cpus, threads, false, NULL) < 0) {
+		if (perf_evsel__open(evsels[i], cpus, threads) < 0) {
 			pr_debug("failed to open counter: %s, "
 				 "tweak /proc/sys/kernel/perf_event_paranoid?\n",
 				 strerror(errno));
@@ -739,7 +739,7 @@ static int test__PERF_RECORD(void)
 	 * Call sys_perf_event_open on all the fds on all the evsels,
 	 * grouping them if asked to.
 	 */
-	err = perf_evlist__open(evlist, opts.group);
+	err = perf_evlist__open(evlist);
 	if (err < 0) {
 		pr_debug("perf_evlist__open: %s\n", strerror(errno));
 		goto out_delete_evlist;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index e3cab5f..c5cc2ab 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -875,17 +875,14 @@ static void perf_top__mmap_read(struct perf_top *top)
 
 static void perf_top__start_counters(struct perf_top *top)
 {
-	struct perf_evsel *counter, *first;
+	struct perf_evsel *counter;
 	struct perf_evlist *evlist = top->evlist;
 
-	first = list_entry(evlist->entries.next, struct perf_evsel, node);
+	if (top->group)
+		perf_evlist__group(evlist);
 
 	list_for_each_entry(counter, &evlist->entries, node) {
 		struct perf_event_attr *attr = &counter->attr;
-		struct xyarray *group_fd = NULL;
-
-		if (top->group && counter != first)
-			group_fd = first->fd;
 
 		attr->sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID;
 
@@ -916,8 +913,7 @@ retry_sample_id:
 		attr->sample_id_all = top->sample_id_all_missing ? 0 : 1;
 try_again:
 		if (perf_evsel__open(counter, top->evlist->cpus,
-				     top->evlist->threads, top->group,
-				     group_fd) < 0) {
+				     top->evlist->threads) < 0) {
 			int err = errno;
 
 			if (err == EPERM || err == EACCES) {
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7400fb3..5fa9257 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -108,6 +108,12 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 	evlist->nr_entries += nr_entries;
 }
 
+void perf_evlist__group(struct perf_evlist *evlist)
+{
+	if (evlist->nr_entries)
+		parse_events__group_leader(&evlist->entries);
+}
+
 int perf_evlist__add_default(struct perf_evlist *evlist)
 {
 	struct perf_event_attr attr = {
@@ -757,21 +763,13 @@ void perf_evlist__set_selected(struct perf_evlist *evlist,
 	evlist->selected = evsel;
 }
 
-int perf_evlist__open(struct perf_evlist *evlist, bool group)
+int perf_evlist__open(struct perf_evlist *evlist)
 {
-	struct perf_evsel *evsel, *first;
+	struct perf_evsel *evsel;
 	int err, ncpus, nthreads;
 
-	first = list_entry(evlist->entries.next, struct perf_evsel, node);
-
 	list_for_each_entry(evsel, &evlist->entries, node) {
-		struct xyarray *group_fd = NULL;
-
-		if (group && evsel != first)
-			group_fd = first->fd;
-
-		err = perf_evsel__open(evsel, evlist->cpus, evlist->threads,
-				       group, group_fd);
+		err = perf_evsel__open(evsel, evlist->cpus, evlist->threads);
 		if (err < 0)
 			goto out_err;
 	}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 989bee9..d842931 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -82,7 +82,7 @@ struct perf_evsel *perf_evlist__id2evsel(struct perf_evlist *evlist, u64 id);
 
 union perf_event *perf_evlist__mmap_read(struct perf_evlist *self, int idx);
 
-int perf_evlist__open(struct perf_evlist *evlist, bool group);
+int perf_evlist__open(struct perf_evlist *evlist);
 
 void perf_evlist__config_attrs(struct perf_evlist *evlist,
 			       struct perf_record_opts *opts);
@@ -126,4 +126,5 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 				   struct list_head *list,
 				   int nr_entries);
 
+void perf_evlist__group(struct perf_evlist *evlist);
 #endif /* __PERF_EVLIST_H */
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 876f639..7e8cc31 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -18,7 +18,6 @@
 #include "../../include/linux/perf_event.h"
 
 #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
-#define GROUP_FD(group_fd, cpu) (*(int *)xyarray__entry(group_fd, cpu, 0))
 
 int __perf_evsel__sample_size(u64 sample_type)
 {
@@ -451,6 +450,7 @@ void perf_evsel__delete(struct perf_evsel *evsel)
 {
 	perf_evsel__exit(evsel);
 	close_cgroup(evsel->cgrp);
+	free(evsel->group_name);
 	free(evsel->name);
 	free(evsel);
 }
@@ -526,9 +526,28 @@ int __perf_evsel__read(struct perf_evsel *evsel,
 	return 0;
 }
 
+static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread)
+{
+	struct perf_evsel *leader = evsel->leader;
+	int fd;
+
+	if (!leader)
+		return -1;
+
+	/*
+	 * Leader must be already processed/open,
+	 * if not it's a bug.
+	 */
+	BUG_ON(!leader->fd);
+
+	fd = FD(leader, cpu, thread);
+	BUG_ON(fd == -1);
+
+	return fd;
+}
+
 static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
-			      struct thread_map *threads, bool group,
-			      struct xyarray *group_fds)
+			      struct thread_map *threads)
 {
 	int cpu, thread;
 	unsigned long flags = 0;
@@ -544,13 +563,15 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 	}
 
 	for (cpu = 0; cpu < cpus->nr; cpu++) {
-		int group_fd = group_fds ? GROUP_FD(group_fds, cpu) : -1;
 
 		for (thread = 0; thread < threads->nr; thread++) {
+			int group_fd;
 
 			if (!evsel->cgrp)
 				pid = threads->map[thread];
 
+			group_fd = get_group_fd(evsel, cpu, thread);
+
 			FD(evsel, cpu, thread) = sys_perf_event_open(&evsel->attr,
 								     pid,
 								     cpus->map[cpu],
@@ -560,8 +581,9 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 				goto out_close;
 			}
 
-			if (group && group_fd == -1)
-				group_fd = FD(evsel, cpu, thread);
+			pr_debug("event cpu %d, thread %d, fd %d, group %d\n",
+				 cpu, pid, FD(evsel, cpu, thread),
+				 group_fd);
 		}
 	}
 
@@ -605,8 +627,7 @@ static struct {
 };
 
 int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
-		     struct thread_map *threads, bool group,
-		     struct xyarray *group_fd)
+		     struct thread_map *threads)
 {
 	if (cpus == NULL) {
 		/* Work around old compiler warnings about strict aliasing */
@@ -616,23 +637,19 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 	if (threads == NULL)
 		threads = &empty_thread_map.map;
 
-	return __perf_evsel__open(evsel, cpus, threads, group, group_fd);
+	return __perf_evsel__open(evsel, cpus, threads);
 }
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
-			     struct cpu_map *cpus, bool group,
-			     struct xyarray *group_fd)
+			     struct cpu_map *cpus)
 {
-	return __perf_evsel__open(evsel, cpus, &empty_thread_map.map, group,
-				  group_fd);
+	return __perf_evsel__open(evsel, cpus, &empty_thread_map.map);
 }
 
 int perf_evsel__open_per_thread(struct perf_evsel *evsel,
-				struct thread_map *threads, bool group,
-				struct xyarray *group_fd)
+				struct thread_map *threads)
 {
-	return __perf_evsel__open(evsel, &empty_cpu_map.map, threads, group,
-				  group_fd);
+	return __perf_evsel__open(evsel, &empty_cpu_map.map, threads);
 }
 
 static int perf_event__parse_id_sample(const union perf_event *event, u64 type,
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 67cc503..b39f3db 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -66,6 +66,8 @@ struct perf_evsel {
 		void		*data;
 	} handler;
 	bool 			supported;
+	struct perf_evsel	*leader;
+	char			*group_name;
 };
 
 struct cpu_map;
@@ -105,14 +107,11 @@ void perf_evsel__free_id(struct perf_evsel *evsel);
 void perf_evsel__close_fd(struct perf_evsel *evsel, int ncpus, int nthreads);
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
-			     struct cpu_map *cpus, bool group,
-			     struct xyarray *group_fds);
+			     struct cpu_map *cpus);
 int perf_evsel__open_per_thread(struct perf_evsel *evsel,
-				struct thread_map *threads, bool group,
-				struct xyarray *group_fds);
+				struct thread_map *threads);
 int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
-		     struct thread_map *threads, bool group,
-		     struct xyarray *group_fds);
+		     struct thread_map *threads);
 void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads);
 
 #define perf_evsel__match(evsel, t, c)		\
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f62cbee..40ad468 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -547,14 +547,32 @@ int parse_events_add_pmu(struct list_head **list, int *idx,
 			 pmu_event_name(head_config));
 }
 
-int parse_events__modifier_group(struct list_head *list __used,
-				 char *event_mod __used)
+struct perf_evsel *parse_events__group_leader(struct list_head *list)
 {
-	return 0;
+	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)
+			evsel->leader = leader;
+
+	return leader;
 }
 
-void parse_events__group(char *name __used, struct list_head *list __used)
+int parse_events__modifier_group(struct list_head *list,
+				 char *event_mod)
 {
+	return parse_events__modifier_event(list, event_mod);
+}
+
+void parse_events__group(char *name, struct list_head *list)
+{
+	struct perf_evsel *leader;
+
+	leader = parse_events__group_leader(list);
+	leader->group_name = name ? strdup(name) : NULL;
 }
 
 void parse_events_update_lists(struct list_head *list_event,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index c2f2ed9..fdbc72a 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -92,6 +92,7 @@ int parse_events_add_breakpoint(struct list_head **list, int *idx,
 				void *ptr, char *type);
 int parse_events_add_pmu(struct list_head **list, int *idx,
 			 char *pmu , struct list_head *head_config);
+struct perf_evsel *parse_events__group_leader(struct list_head *list);
 void parse_events__group(char *name, struct list_head *list);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index e03b58a..419c29e 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -627,7 +627,7 @@ static PyObject *pyrf_evsel__open(struct pyrf_evsel *pevsel,
 	 * This will group just the fds for this single evsel, to group
 	 * multiple events, use evlist.open().
 	 */
-	if (perf_evsel__open(evsel, cpus, threads, group, NULL) < 0) {
+	if (perf_evsel__open(evsel, cpus, threads) < 0) {
 		PyErr_SetFromErrno(PyExc_OSError);
 		return NULL;
 	}
@@ -828,7 +828,10 @@ static PyObject *pyrf_evlist__open(struct pyrf_evlist *pevlist,
 	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|OOii", kwlist, &group))
 		return NULL;
 
-	if (perf_evlist__open(evlist, group) < 0) {
+	if (group)
+		perf_evlist__group(evlist);
+
+	if (perf_evlist__open(evlist) < 0) {
 		PyErr_SetFromErrno(PyExc_OSError);
 		return NULL;
 	}
-- 
1.7.7.6

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Arnaldo Carvalho de Melo]

Em Fri, Jun 29, 2012 at 11:08:26AM +0200, Jiri Olsa escreveu:
> This patch adds a functionality that allows to create event groups
> based on the way they are specified on the command line. Adding
> functionality to the '{}' group syntax introduced in earlier patch.
> 
> The current '--group/-g' option behaviour remains intact.  If you
> specify it for record/stat/top command, all the specified events
> become members of a single group with the first event as a group
> leader.
> 
> With the new '{}' group syntax you can create group like:
>   # perf record -e '{cycles,faults}' ls

All this cset comment makes for great tools/perf/Documentation material,
right? Can you try and send a patch to add this to the 'record' man
page?

On top of these, that I'm merging/testing now.

- Arnaldo

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Jiri Olsa]

On Fri, Jun 29, 2012 at 01:47:36PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 29, 2012 at 11:08:26AM +0200, Jiri Olsa escreveu:
> > This patch adds a functionality that allows to create event groups
> > based on the way they are specified on the command line. Adding
> > functionality to the '{}' group syntax introduced in earlier patch.
> > 
> > The current '--group/-g' option behaviour remains intact.  If you
> > specify it for record/stat/top command, all the specified events
> > become members of a single group with the first event as a group
> > leader.
> > 
> > With the new '{}' group syntax you can create group like:
> >   # perf record -e '{cycles,faults}' ls
> 
> All this cset comment makes for great tools/perf/Documentation material,
> right? Can you try and send a patch to add this to the 'record' man
> page?

ok, I'll send doc patch with the next patchset

jirka

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Namhyung Kim]

On Fri, 29 Jun 2012 11:08:26 +0200, Jiri Olsa wrote:
> This patch adds a functionality that allows to create event groups
> based on the way they are specified on the command line. Adding
> functionality to the '{}' group syntax introduced in earlier patch.
>
> The current '--group/-g' option behaviour remains intact.  If you
> specify it for record/stat/top command, all the specified events
> become members of a single group with the first event as a group
> leader.
>
> With the new '{}' group syntax you can create group like:
>   # perf record -e '{cycles,faults}' ls
>
> resulting in single event group containing 'cycles' and 'faults'
> events, with cycles event as group leader.
>
> All groups are created with regards to threads and cpus. Thus
> recording an event group within a 2 threads on server with
> 4 CPUs will create 8 separate groups.
>
> Examples (first event in brackets is group leader):
>
>   # 1 group (cpu-clock,task-clock)
>   perf record --group -e cpu-clock,task-clock ls
>   perf record -e '{cpu-clock,task-clock}' ls
>
>   # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
>   perf record -e '{cpu-clock,task-clock},{minor-faults,major-faults}' ls
>
>   # 1 group (cpu-clock,task-clock,minor-faults,major-faults)
>   perf record --group -e cpu-clock,task-clock -e minor-faults,major-faults ls
>   perf record -e '{cpu-clock,task-clock,minor-faults,major-faults}' ls
>
>   # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
>   perf record -e '{cpu-clock,task-clock} -e '{minor-faults,major-faults}' \
>    -e instructions ls
>
>   # 1 group (cpu-clock,task-clock,minor-faults,major-faults,instructions)
>   perf record --group -e cpu-clock,task-clock \
>    -e minor-faults,major-faults -e instructions ls
>   perf record -e '{cpu-clock,task-clock,minor-faults,major-faults,instructions}' ls
>
> It's possible to use standard event modifier for a group, which spans
> over all events in the group and overrides any event modifier settings,
> for example:
>
>   # perf record -r '{faults:k,cache-references}:u'
>
> resulting in ':u' modifier being used for both 'faults' and 'cache-references'
> events, regardless of their modifier setup (':k' for faults event).
>

So the faults event would get only 'u' for the modifier not 'uk', right?

Thanks,
Namhyung

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Jiri Olsa]

On Mon, Jul 02, 2012 at 11:13:56AM +0900, Namhyung Kim wrote:
> On Fri, 29 Jun 2012 11:08:26 +0200, Jiri Olsa wrote:
> > This patch adds a functionality that allows to create event groups
> > based on the way they are specified on the command line. Adding
> > functionality to the '{}' group syntax introduced in earlier patch.
> >
> > The current '--group/-g' option behaviour remains intact.  If you
> > specify it for record/stat/top command, all the specified events
> > become members of a single group with the first event as a group
> > leader.
> >
> > With the new '{}' group syntax you can create group like:
> >   # perf record -e '{cycles,faults}' ls
> >
> > resulting in single event group containing 'cycles' and 'faults'
> > events, with cycles event as group leader.
> >
> > All groups are created with regards to threads and cpus. Thus
> > recording an event group within a 2 threads on server with
> > 4 CPUs will create 8 separate groups.
> >
> > Examples (first event in brackets is group leader):
> >
> >   # 1 group (cpu-clock,task-clock)
> >   perf record --group -e cpu-clock,task-clock ls
> >   perf record -e '{cpu-clock,task-clock}' ls
> >
> >   # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
> >   perf record -e '{cpu-clock,task-clock},{minor-faults,major-faults}' ls
> >
> >   # 1 group (cpu-clock,task-clock,minor-faults,major-faults)
> >   perf record --group -e cpu-clock,task-clock -e minor-faults,major-faults ls
> >   perf record -e '{cpu-clock,task-clock,minor-faults,major-faults}' ls
> >
> >   # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
> >   perf record -e '{cpu-clock,task-clock} -e '{minor-faults,major-faults}' \
> >    -e instructions ls
> >
> >   # 1 group (cpu-clock,task-clock,minor-faults,major-faults,instructions)
> >   perf record --group -e cpu-clock,task-clock \
> >    -e minor-faults,major-faults -e instructions ls
> >   perf record -e '{cpu-clock,task-clock,minor-faults,major-faults,instructions}' ls
> >
> > It's possible to use standard event modifier for a group, which spans
> > over all events in the group and overrides any event modifier settings,
> > for example:
> >
> >   # perf record -r '{faults:k,cache-references}:u'
> >
> > resulting in ':u' modifier being used for both 'faults' and 'cache-references'
> > events, regardless of their modifier setup (':k' for faults event).
> >
> 
> So the faults event would get only 'u' for the modifier not 'uk', right?

right

jirka

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Namhyung Kim]

On Mon, 2 Jul 2012 12:10:10 +0200, Jiri Olsa wrote:
> On Mon, Jul 02, 2012 at 11:13:56AM +0900, Namhyung Kim wrote:
>> On Fri, 29 Jun 2012 11:08:26 +0200, Jiri Olsa wrote:
>> > It's possible to use standard event modifier for a group, which spans
>> > over all events in the group and overrides any event modifier settings,
>> > for example:
>> >
>> >   # perf record -r '{faults:k,cache-references}:u'
>> >
>> > resulting in ':u' modifier being used for both 'faults' and 'cache-references'
>> > events, regardless of their modifier setup (':k' for faults event).
>> >
>> 
>> So the faults event would get only 'u' for the modifier not 'uk', right?
>
> right
>

I was thinking about the combining the modifiers - i.e. make it 'uk'
above. Not sure it's useful in general, but something like "adding 'p'
to the leader (only)" looks good to have IMHO.

Thanks,
Namhyung

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Jiri Olsa]

On Tue, Jul 03, 2012 at 09:58:44AM +0900, Namhyung Kim wrote:
> On Mon, 2 Jul 2012 12:10:10 +0200, Jiri Olsa wrote:
> > On Mon, Jul 02, 2012 at 11:13:56AM +0900, Namhyung Kim wrote:
> >> On Fri, 29 Jun 2012 11:08:26 +0200, Jiri Olsa wrote:
> >> > It's possible to use standard event modifier for a group, which spans
> >> > over all events in the group and overrides any event modifier settings,
> >> > for example:
> >> >
> >> >   # perf record -r '{faults:k,cache-references}:u'
> >> >
> >> > resulting in ':u' modifier being used for both 'faults' and 'cache-references'
> >> > events, regardless of their modifier setup (':k' for faults event).
> >> >
> >> 
> >> So the faults event would get only 'u' for the modifier not 'uk', right?
> >
> > right
> >
> 
> I was thinking about the combining the modifiers - i.e. make it 'uk'
> above. Not sure it's useful in general, but something like "adding 'p'
> to the leader (only)" looks good to have IMHO.

just hit the issue with leader 'p' during tests ;) but still not sure which
way is more usefull thought.. probably better to have both ways available

how about uppercase group modifier overwrites, lowercase means combining

or any other way.. ;) thoughts?

jirka

> 
> Thanks,
> Namhyung

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Namhyung Kim]

On Wed, 4 Jul 2012 19:34:07 +0200, Jiri Olsa wrote:
> On Tue, Jul 03, 2012 at 09:58:44AM +0900, Namhyung Kim wrote:
>> On Mon, 2 Jul 2012 12:10:10 +0200, Jiri Olsa wrote:
>> > On Mon, Jul 02, 2012 at 11:13:56AM +0900, Namhyung Kim wrote:
>> >> On Fri, 29 Jun 2012 11:08:26 +0200, Jiri Olsa wrote:
>> >> > It's possible to use standard event modifier for a group, which spans
>> >> > over all events in the group and overrides any event modifier settings,
>> >> > for example:
>> >> >
>> >> >   # perf record -r '{faults:k,cache-references}:u'
>> >> >
>> >> > resulting in ':u' modifier being used for both 'faults' and 'cache-references'
>> >> > events, regardless of their modifier setup (':k' for faults event).
>> >> >
>> >> 
>> >> So the faults event would get only 'u' for the modifier not 'uk', right?
>> >
>> > right
>> >
>> 
>> I was thinking about the combining the modifiers - i.e. make it 'uk'
>> above. Not sure it's useful in general, but something like "adding 'p'
>> to the leader (only)" looks good to have IMHO.
>
> just hit the issue with leader 'p' during tests ;) but still not sure which
> way is more usefull thought.. probably better to have both ways available
>
> how about uppercase group modifier overwrites, lowercase means combining
>
> or any other way.. ;) thoughts?
>

How about using like '{...}+u' for combining?

Thanks,
Namhyung

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Peter Zijlstra]

On Wed, 2012-07-04 at 19:34 +0200, Jiri Olsa wrote:
> or any other way.. ;) thoughts?

What again was the downside if always doing additive (instead of
overriding) attributes?

Re: [PATCH 2/3] perf, tool: Enable grouping logic for parsed events

[Arnaldo Carvalho de Melo]

Em Thu, Jul 05, 2012 at 06:05:51PM +0200, Peter Zijlstra escreveu:
> On Wed, 2012-07-04 at 19:34 +0200, Jiri Olsa wrote:
> > or any other way.. ;) thoughts?
> 
> What again was the downside if always doing additive (instead of
> overriding) attributes?

Yeah, I think this is the sanest way: additive. Overriding is confusing.

- Arnaldo

[PATCH 3/3] perf, test: Add automated tests for event group parsing

[Jiri Olsa]

Adding 3 more tests for new event group syntax. Tests are executed
within the 'perf test parse' test suite.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/parse-events-test.c |  152 +++++++++++++++++++++++++++++++++++
 1 files changed, 152 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/parse-events-test.c b/tools/perf/util/parse-events-test.c
index 229af6d..66e6cc1 100644
--- a/tools/perf/util/parse-events-test.c
+++ b/tools/perf/util/parse-events-test.c
@@ -473,6 +473,146 @@ static int test__checkterms_simple(struct list_head *terms)
 	return 0;
 }
 
+static int test__group1(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel, *leader;
+
+	TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
+
+	/* instructions:k */
+	evsel = leader = list_entry(evlist->entries.next,
+				    struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_HW_INSTRUCTIONS == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude_user", evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	/* cycles:upp */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_HW_CPU_CYCLES == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude_user", !evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", evsel->attr.precise_ip == 2);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == leader);
+
+	return 0;
+}
+
+static int test__group2(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel, *leader;
+
+	TEST_ASSERT_VAL("wrong number of entries", 3 == evlist->nr_entries);
+
+	/* faults + :u modifier */
+	evsel = leader = list_entry(evlist->entries.next,
+				    struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_SOFTWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_SW_PAGE_FAULTS == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude_user", !evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	/* cache-references + :u modifier */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_HW_CACHE_REFERENCES == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude_user", !evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == leader);
+
+	/* cycles:k */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_HW_CPU_CYCLES == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude_user", evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	return 0;
+}
+
+static int test__group3(struct perf_evlist *evlist __used)
+{
+	struct perf_evsel *evsel, *leader;
+
+	TEST_ASSERT_VAL("wrong number of entries", 5 == evlist->nr_entries);
+
+	/* group1 syscalls:sys_enter_open:H */
+	evsel = leader = list_entry(evlist->entries.next,
+				    struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_TRACEPOINT == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong sample_type",
+		(PERF_SAMPLE_RAW | PERF_SAMPLE_TIME | PERF_SAMPLE_CPU) ==
+		evsel->attr.sample_type);
+	TEST_ASSERT_VAL("wrong sample_period", 1 == evsel->attr.sample_period);
+	TEST_ASSERT_VAL("wrong exclude guest", evsel->attr.exclude_guest);
+	TEST_ASSERT_VAL("wrong exclude host", !evsel->attr.exclude_host);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+	TEST_ASSERT_VAL("wrong group name",
+		!strcmp(leader->group_name, "group1"));
+
+	/* group1 cycles:kppp */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_HW_CPU_CYCLES == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude_user", evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", evsel->attr.precise_ip == 3);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == leader);
+	TEST_ASSERT_VAL("wrong group name", !evsel->group_name);
+
+	/* group2 cycles + G modifier */
+	evsel = leader = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_HW_CPU_CYCLES == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
+	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+	TEST_ASSERT_VAL("wrong group name",
+		!strcmp(leader->group_name, "group2"));
+
+	/* group2 1:3 + G modifier */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", 1 == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config", 3 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude guest", !evsel->attr.exclude_guest);
+	TEST_ASSERT_VAL("wrong exclude host", evsel->attr.exclude_host);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == leader);
+
+	/* instructions:u */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",
+			PERF_COUNT_HW_INSTRUCTIONS == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong exclude_user", !evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	return 0;
+}
+
 struct test__event_st {
 	const char *name;
 	__u32 type;
@@ -584,6 +724,18 @@ static struct test__event_st test__events[] = {
 		.name  = "instructions:H",
 		.check = test__checkevent_exclude_guest_modifier,
 	},
+	[26] = {
+		.name  = "{instructions:k,cycles:upp}",
+		.check = test__group1,
+	},
+	[27] = {
+		.name  = "{faults:k,cache-references}:u,cycles:k",
+		.check = test__group2,
+	},
+	[28] = {
+		.name  = "group1{syscalls:sys_enter_open:H,cycles:kppp},group2{cycles,1:3}:G,instructions:u",
+		.check = test__group3,
+	},
 };
 
 #define TEST__EVENTS_CNT (sizeof(test__events) / sizeof(struct test__event_st))
-- 
1.7.7.6

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Namhyung Kim]

Hi, Jiri

On Fri, 29 Jun 2012 11:08:24 +0200, Jiri Olsa wrote:
> hi,
> adding support for creating event groups based on the way they
> are specified on the command line.
>
> This patchset adds the '{}' style grammar to express event group,
> allowing so far only the 'event modifier' as group modifier.
>
> The sample/leader modifier changes are ready, but I'm getting
> some weird numbers in tests, so I'll send it later. Meanwhile,
> it'd be nice to have at least this patchset in.. ;)
>

Just a question, is there a way to know about the grouping at perf
report time?

Thanks,
Namhyung


>
> Attached patches:
>   1/3 perf, tool: Add support to parse event group syntax
>   2/3 perf, tool: Enable grouping logic for parsed events
>   3/3 perf, test: Add automated tests for event group parsing
>
> wbr,
> jirka
> ---
>  tools/perf/builtin-record.c         |   13 +--
>  tools/perf/builtin-stat.c           |   13 +--
>  tools/perf/builtin-test.c           |    8 +-
>  tools/perf/builtin-top.c            |   12 +--
>  tools/perf/util/evlist.c            |   20 ++---
>  tools/perf/util/evlist.h            |    3 +-
>  tools/perf/util/evsel.c             |   51 ++++++++----
>  tools/perf/util/evsel.h             |   11 +--
>  tools/perf/util/parse-events-test.c |  152 +++++++++++++++++++++++++++++++++++
>  tools/perf/util/parse-events.c      |   32 +++++++-
>  tools/perf/util/parse-events.h      |    5 +-
>  tools/perf/util/parse-events.l      |    2 +
>  tools/perf/util/parse-events.y      |   93 ++++++++++++++++++---
>  tools/perf/util/python.c            |    7 +-
>  14 files changed, 341 insertions(+), 81 deletions(-)

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Jiri Olsa]

On Mon, Jul 02, 2012 at 10:53:44AM +0900, Namhyung Kim wrote:
> Hi, Jiri
> 
> On Fri, 29 Jun 2012 11:08:24 +0200, Jiri Olsa wrote:
> > hi,
> > adding support for creating event groups based on the way they
> > are specified on the command line.
> >
> > This patchset adds the '{}' style grammar to express event group,
> > allowing so far only the 'event modifier' as group modifier.
> >
> > The sample/leader modifier changes are ready, but I'm getting
> > some weird numbers in tests, so I'll send it later. Meanwhile,
> > it'd be nice to have at least this patchset in.. ;)
> >
> 
> Just a question, is there a way to know about the grouping at perf
> report time?

nope, AFAIK only ID and perf_event_attr is stored for event
grouping is known only for record time

jirka

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Namhyung Kim]

2012-07-02 (월), 12:15 +0200, Jiri Olsa:
> On Mon, Jul 02, 2012 at 10:53:44AM +0900, Namhyung Kim wrote:
> > Just a question, is there a way to know about the grouping at perf
> > report time?
> 
> nope, AFAIK only ID and perf_event_attr is stored for event
> grouping is known only for record time
> 

I heard that Arnaldo (or Stephane) wanted to make perf report
group-aware or such so that it can show related events together. But to
do that, it seems we need to change the data file format first, right?

Any idea?


-- 
Regards,
Namhyung Kim

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Jiri Olsa]

On Mon, Jul 02, 2012 at 10:11:02PM +0900, Namhyung Kim wrote:
> 2012-07-02 (월), 12:15 +0200, Jiri Olsa:
> > On Mon, Jul 02, 2012 at 10:53:44AM +0900, Namhyung Kim wrote:
> > > Just a question, is there a way to know about the grouping at perf
> > > report time?
> > 
> > nope, AFAIK only ID and perf_event_attr is stored for event
> > grouping is known only for record time
> > 
> 
> I heard that Arnaldo (or Stephane) wanted to make perf report
> group-aware or such so that it can show related events together. But to
> do that, it seems we need to change the data file format first, right?
> 
> Any idea?

The next change I'm working on is to record and report
PERF_FORMAT_GROUP related data. Here's commit comment
from my next patchset:

---
perf, tool: Enable sampling on specified event group leader

Adding the functionality to the group modifier event syntax.
Allowing user to select leader event inside the group using
event index (command line event position in the group).

Following example selects e2 as leader:
  -e '{e1,e2,e3,e4}:2'

The selected event becomes group leader and is the only one
doing samples.

The rest of the events in the group are being read on each leader
event sample by PERF_SAMPLE_READ sample type processing.

Following example:
  perf record -e {cycles,faults}:1 ls

  - creates a group with 'cycles' and 'faults' events
  - 'cycles' event is group leader and has sampling enabled
  - 'faults' event is read each time 'cycles' sample,
    the 'faults' count is attached to the 'cycles sample
    via PERF_SAMPLE_READ sample type.
---

The report does not need any new metadata about grouping, because
the samples are generated/stored only from the group leader. The
other events data are read from the PERF_FORMAT_GROUP leader sample
data.

So no data file format change for my next changes, but I'm not sure
this is the report change you mean.

jirka

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Namhyung Kim]

2012-07-02 (월), 15:33 +0200, Jiri Olsa:
> On Mon, Jul 02, 2012 at 10:11:02PM +0900, Namhyung Kim wrote:
> > 2012-07-02 (월), 12:15 +0200, Jiri Olsa:
> > > On Mon, Jul 02, 2012 at 10:53:44AM +0900, Namhyung Kim wrote:
> > > > Just a question, is there a way to know about the grouping at perf
> > > > report time?
> > > 
> > > nope, AFAIK only ID and perf_event_attr is stored for event
> > > grouping is known only for record time
> > > 
> > 
> > I heard that Arnaldo (or Stephane) wanted to make perf report
> > group-aware or such so that it can show related events together. But to
> > do that, it seems we need to change the data file format first, right?
> > 
> > Any idea?
> 
> The next change I'm working on is to record and report
> PERF_FORMAT_GROUP related data. Here's commit comment
> from my next patchset:
> 
> ---
> perf, tool: Enable sampling on specified event group leader
> 
> Adding the functionality to the group modifier event syntax.
> Allowing user to select leader event inside the group using
> event index (command line event position in the group).
> 
> Following example selects e2 as leader:
>   -e '{e1,e2,e3,e4}:2'
> 
> The selected event becomes group leader and is the only one
> doing samples.
> 
> The rest of the events in the group are being read on each leader
> event sample by PERF_SAMPLE_READ sample type processing.
> 
> Following example:
>   perf record -e {cycles,faults}:1 ls
> 
>   - creates a group with 'cycles' and 'faults' events
>   - 'cycles' event is group leader and has sampling enabled
>   - 'faults' event is read each time 'cycles' sample,
>     the 'faults' count is attached to the 'cycles sample
>     via PERF_SAMPLE_READ sample type.
> ---
> 
> The report does not need any new metadata about grouping, because
> the samples are generated/stored only from the group leader. The
> other events data are read from the PERF_FORMAT_GROUP leader sample
> data.
> 
> So no data file format change for my next changes, but I'm not sure
> this is the report change you mean.
> 

What I said is just displaying cycles and faults events in above example
on the same screen/table like:

 (Overhead)
cycles  faults  Command      Shared object              Symbol
......  ......  .......  .................  ..................
92.98%  60.13%  noploop  noploop            [.] main
 3.21%   5.42%  noploop  [kernel.kallsyms]  [k] __lock_acquire
 1.16%  13.13%  noploop  libc-2.11.1.so     [.] _int_malloc
 0.97%   0.33%  noploop  [kernel.kallsyms]  [k] clear_page_c
...

We could pass such information to perf report explicitly, But I guess it
'd better if perf report did it for me automagically by detecting group
relations.


-- 
Regards,
Namhyung Kim

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Namhyung Kim]

On Mon, 02 Jul 2012 23:20:34 +0900, Namhyung Kim wrote:
> We could pass such information to perf report explicitly, But I guess it
> 'd better if perf report did it for me automagically by detecting group
> relations.

Oh I think we can do it by parsing the command line (again) at the perf
report time. In that way, no need to change the file format. I'm gonna
give it a shot later.

Thanks,
Namhyung

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Arnaldo Carvalho de Melo]

Em Tue, Jul 03, 2012 at 09:50:19AM +0900, Namhyung Kim escreveu:
> On Mon, 02 Jul 2012 23:20:34 +0900, Namhyung Kim wrote:
> > We could pass such information to perf report explicitly, But I guess it
> > 'd better if perf report did it for me automagically by detecting group
> > relations.
> 
> Oh I think we can do it by parsing the command line (again) at the perf
> report time. In that way, no need to change the file format. I'm gonna
> give it a shot later.

Its there already...

[acme@felicio ~]$ perf record -e cycles,cache-misses sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.016 MB perf.data (~713 samples) ]
[acme@felicio ~]$ perf report --stdio | head -20
# ========
# captured on: Mon Jul  2 22:02:00 2012
# hostname : felicio.ghostprotocols.net
# os release : 3.4.0-rc5+
# perf version : 3.5.rc1.104.gd3076383
# arch : x86_64
# nrcpus online : 4
# nrcpus avail : 4
# cpudesc : Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz
# cpuid : GenuineIntel,6,42,7
# total memory : 8089800 kB
# cmdline : /home/acme/bin/perf record -e cycles,cache-misses sleep 1 
# event : name = cycles, type = 0, config = 0x0, config1 = 0x0, config2
# = 0x0, excl_usr = 0, excl_kern = 0, id = { 10, 11, 12, 13 }
# event : name = cache-misses, type = 0, config = 0x3, config1 = 0x0,
# config2 = 0x0, excl_usr = 0, excl_kern = 0, id = { 14, 15, 16, 17 }
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
# ========
#
# Samples: 19  of event 'cycles'
# Event count (approx.): 2442170
[acme@felicio ~]$ 

I.e. for perf report it is ok already, the user can see what was asked
for, exactly as specified on the command line.

What I discussed with Jiri was that it would be good to have a
programatic way of regenerating the perf_evlist instance from the
perf_event_attr and other feature bits.

I.e. the counterpart to perf_evsel being created from the
perf_event_attr in the perf.data file.

- Arnaldo

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Stephane Eranian]

On Mon, Jul 2, 2012 at 3:33 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Mon, Jul 02, 2012 at 10:11:02PM +0900, Namhyung Kim wrote:
>> 2012-07-02 (월), 12:15 +0200, Jiri Olsa:
>> > On Mon, Jul 02, 2012 at 10:53:44AM +0900, Namhyung Kim wrote:
>> > > Just a question, is there a way to know about the grouping at perf
>> > > report time?
>> >
>> > nope, AFAIK only ID and perf_event_attr is stored for event
>> > grouping is known only for record time
>> >
>>
>> I heard that Arnaldo (or Stephane) wanted to make perf report
>> group-aware or such so that it can show related events together. But to
>> do that, it seems we need to change the data file format first, right?
>>
>> Any idea?
>
> The next change I'm working on is to record and report
> PERF_FORMAT_GROUP related data. Here's commit comment
> from my next patchset:
>
> ---
> perf, tool: Enable sampling on specified event group leader
>
> Adding the functionality to the group modifier event syntax.
> Allowing user to select leader event inside the group using
> event index (command line event position in the group).
>
> Following example selects e2 as leader:
>   -e '{e1,e2,e3,e4}:2'
>
I don't understand why you actually need the :2 suffix. There can
only be one leader. So assume it is the first one. Users have to
know the first one is the leader which seems like a natural thing
to do for me. It would make you syntax less ugly than it already
is.

I would have thought you could enable this with a simple
cmdline option which changes the way you interpret the
multiple -e options:

perf record --group-reads -e e1,e2,e3 -e e4,e5,e6 .....

Would setup the group leaders (e1, e4) for 2 groups.
NO curly braces, no : needed.

> The selected event becomes group leader and is the only one
> doing samples.
>
> The rest of the events in the group are being read on each leader
> event sample by PERF_SAMPLE_READ sample type processing.
>
> Following example:
>   perf record -e {cycles,faults}:1 ls
>
>   - creates a group with 'cycles' and 'faults' events
>   - 'cycles' event is group leader and has sampling enabled
>   - 'faults' event is read each time 'cycles' sample,
>     the 'faults' count is attached to the 'cycles sample
>     via PERF_SAMPLE_READ sample type.
> ---
>
> The report does not need any new metadata about grouping, because
> the samples are generated/stored only from the group leader. The
> other events data are read from the PERF_FORMAT_GROUP leader sample
> data.
>
> So no data file format change for my next changes, but I'm not sure
> this is the report change you mean.
>
> jirka

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Arnaldo Carvalho de Melo]

Em Thu, Jul 05, 2012 at 06:15:20PM +0200, Stephane Eranian escreveu:
> On Mon, Jul 2, 2012 at 3:33 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> > perf, tool: Enable sampling on specified event group leader
> >
> > Adding the functionality to the group modifier event syntax.
> > Allowing user to select leader event inside the group using
> > event index (command line event position in the group).
> >
> > Following example selects e2 as leader:
> >   -e '{e1,e2,e3,e4}:2'
> >
> I don't understand why you actually need the :2 suffix. There can
> only be one leader. So assume it is the first one. Users have to
> know the first one is the leader which seems like a natural thing
> to do for me. It would make you syntax less ugly than it already
> is.

Agreed, looks like creeping featurism.
 
> I would have thought you could enable this with a simple
> cmdline option which changes the way you interpret the
> multiple -e options:
> 
> perf record --group-reads -e e1,e2,e3 -e e4,e5,e6 .....
> 
> Would setup the group leaders (e1, e4) for 2 groups.
> NO curly braces, no : needed.

Yeah, curly braces needed just when one wants to add group wide
modifiers.
 
- Arnaldo

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Stephane Eranian]

On Thu, Jul 5, 2012 at 6:44 PM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Thu, Jul 05, 2012 at 06:15:20PM +0200, Stephane Eranian escreveu:
>> On Mon, Jul 2, 2012 at 3:33 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>> > perf, tool: Enable sampling on specified event group leader
>> >
>> > Adding the functionality to the group modifier event syntax.
>> > Allowing user to select leader event inside the group using
>> > event index (command line event position in the group).
>> >
>> > Following example selects e2 as leader:
>> >   -e '{e1,e2,e3,e4}:2'
>> >
>> I don't understand why you actually need the :2 suffix. There can
>> only be one leader. So assume it is the first one. Users have to
>> know the first one is the leader which seems like a natural thing
>> to do for me. It would make you syntax less ugly than it already
>> is.
>
> Agreed, looks like creeping featurism.
>
>> I would have thought you could enable this with a simple
>> cmdline option which changes the way you interpret the
>> multiple -e options:
>>
>> perf record --group-reads -e e1,e2,e3 -e e4,e5,e6 .....
>>
>> Would setup the group leaders (e1, e4) for 2 groups.
>> NO curly braces, no : needed.
>
> Yeah, curly braces needed just when one wants to add group wide
> modifiers.
And even, in that case that seems overkill too.
You can simply repeat the modifier on each event. A group cannot
big that big. It is limited in size by the number of physical counters
plus the events constraints.

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Ulrich Drepper]

On Thu, Jul 5, 2012 at 12:15 PM, Stephane Eranian <eranian@google.com> wrote:
> I don't understand why you actually need the :2 suffix. There can
> only be one leader. So assume it is the first one. Users have to
> know the first one is the leader which seems like a natural thing
> to do for me. It would make you syntax less ugly than it already
> is.

In a blue sky world I would have done this.  In fact, this is what I
tried before reading the sources to find out there is no group support
so far.  But given that multiple -e options already have a meaning I
would be hesitant to change this.

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Stephane Eranian]

On Fri, Jul 6, 2012 at 3:32 AM, Ulrich Drepper <drepper@gmail.com> wrote:
> On Thu, Jul 5, 2012 at 12:15 PM, Stephane Eranian <eranian@google.com> wrote:
>> I don't understand why you actually need the :2 suffix. There can
>> only be one leader. So assume it is the first one. Users have to
>> know the first one is the leader which seems like a natural thing
>> to do for me. It would make you syntax less ugly than it already
>> is.
>
> In a blue sky world I would have done this.  In fact, this is what I
> tried before reading the sources to find out there is no group support
> so far.  But given that multiple -e options already have a meaning I
> would be hesitant to change this.

That's why I said you activate grouping via -e only when you have
the --group-events or --group-reads option in front. That would
not change the meaning of the multiple -e when none of those
group options are specified.

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Jiri Olsa]

On Fri, Jul 06, 2012 at 03:42:54AM +0200, Stephane Eranian wrote:
> On Fri, Jul 6, 2012 at 3:32 AM, Ulrich Drepper <drepper@gmail.com> wrote:
> > On Thu, Jul 5, 2012 at 12:15 PM, Stephane Eranian <eranian@google.com> wrote:
> >> I don't understand why you actually need the :2 suffix. There can
> >> only be one leader. So assume it is the first one. Users have to
> >> know the first one is the leader which seems like a natural thing
> >> to do for me. It would make you syntax less ugly than it already
> >> is.
> >
> > In a blue sky world I would have done this.  In fact, this is what I
> > tried before reading the sources to find out there is no group support
> > so far.  But given that multiple -e options already have a meaning I
> > would be hesitant to change this.
> 
> That's why I said you activate grouping via -e only when you have
> the --group-events or --group-reads option in front. That would
> not change the meaning of the multiple -e when none of those
> group options are specified.

I discussed this with peter..

<peterz> the {} thing allows: 1) multiple groups in a single -e, 2) group attributes 

as for the leader sampling, we can have the first event to become the leader
by default (omit the leader index modifier) and enable the leader sampling by
another modifier:

<peterz> right, just make it a single 'l' (el not one) to indicate 'leader' sampling


jirka

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Peter Zijlstra]

On Mon, 2012-07-09 at 13:05 +0200, Jiri Olsa wrote:
> <peterz> the {} thing allows: 1) multiple groups in a single -e

The advantage would be when you're passing such things on via
environment variables or somesuch.

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Stephane Eranian]

On Mon, Jul 9, 2012 at 1:05 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Jul 06, 2012 at 03:42:54AM +0200, Stephane Eranian wrote:
> > On Fri, Jul 6, 2012 at 3:32 AM, Ulrich Drepper <drepper@gmail.com> wrote:
> > > On Thu, Jul 5, 2012 at 12:15 PM, Stephane Eranian <eranian@google.com> wrote:
> > >> I don't understand why you actually need the :2 suffix. There can
> > >> only be one leader. So assume it is the first one. Users have to
> > >> know the first one is the leader which seems like a natural thing
> > >> to do for me. It would make you syntax less ugly than it already
> > >> is.
> > >
> > > In a blue sky world I would have done this.  In fact, this is what I
> > > tried before reading the sources to find out there is no group support
> > > so far.  But given that multiple -e options already have a meaning I
> > > would be hesitant to change this.
> >
> > That's why I said you activate grouping via -e only when you have
> > the --group-events or --group-reads option in front. That would
> > not change the meaning of the multiple -e when none of those
> > group options are specified.
>
> I discussed this with peter..
>
> <peterz> the {} thing allows: 1) multiple groups in a single -e, 2) group attributes
>
And what's the value of 1) exactly? What's wrong with passing multiple -e ?
The only group attribute I can think of would be :u, :k. Not so much typing.

> as for the leader sampling, we can have the first event to become the leader
> by default (omit the leader index modifier) and enable the leader sampling by
> another modifier:
>
I don't understand this sentence.

> <peterz> right, just make it a single 'l' (el not one) to indicate 'leader' sampling
>
To me ,this looks a bit of an over-engineered design and it is not based on
any actual user requests. Don't get me wrong, grouping is useful and required
but nobody has ever asked for that level of flexibility. The syntax you have
now is already very rich for my taste.




>
> jirka

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Andi Kleen]

> To me ,this looks a bit of an over-engineered design and it is not based on
> any actual user requests. Don't get me wrong, grouping is useful and required
> but nobody has ever asked for that level of flexibility. The syntax you have
> now is already very rich for my taste.

The user input I hear is just get the basic support working ASAP.

-Andi

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Jiri Olsa]

On Tue, Jul 17, 2012 at 09:15:23AM +0200, Stephane Eranian wrote:
> On Mon, Jul 9, 2012 at 1:05 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Fri, Jul 06, 2012 at 03:42:54AM +0200, Stephane Eranian wrote:
> > > On Fri, Jul 6, 2012 at 3:32 AM, Ulrich Drepper <drepper@gmail.com> wrote:
> > > > On Thu, Jul 5, 2012 at 12:15 PM, Stephane Eranian <eranian@google.com> wrote:
> > > >> I don't understand why you actually need the :2 suffix. There can
> > > >> only be one leader. So assume it is the first one. Users have to
> > > >> know the first one is the leader which seems like a natural thing
> > > >> to do for me. It would make you syntax less ugly than it already
> > > >> is.
> > > >
> > > > In a blue sky world I would have done this.  In fact, this is what I
> > > > tried before reading the sources to find out there is no group support
> > > > so far.  But given that multiple -e options already have a meaning I
> > > > would be hesitant to change this.
> > >
> > > That's why I said you activate grouping via -e only when you have
> > > the --group-events or --group-reads option in front. That would
> > > not change the meaning of the multiple -e when none of those
> > > group options are specified.
> >
> > I discussed this with peter..
> >
> > <peterz> the {} thing allows: 1) multiple groups in a single -e, 2) group attributes
> >
> And what's the value of 1) exactly? What's wrong with passing multiple -e ?
> The only group attribute I can think of would be :u, :k. Not so much typing.
> 
> > as for the leader sampling, we can have the first event to become the leader
> > by default (omit the leader index modifier) and enable the leader sampling by
> > another modifier:
> >
> I don't understand this sentence.
> 
> > <peterz> right, just make it a single 'l' (el not one) to indicate 'leader' sampling
> >
> To me ,this looks a bit of an over-engineered design and it is not based on
> any actual user requests. Don't get me wrong, grouping is useful and required
> but nobody has ever asked for that level of flexibility. The syntax you have
> now is already very rich for my taste.

Well, I personally like the '{}' syntax more than '--group-events or --group-reads
option in front', it feels more user friendly.. anyway, we can easily have both ways.

As for the group attributes and group leader sampling, I don't mind omitting
them at this point and get back to that if we find it useful in future.

jirka

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Ulrich Drepper]

On Wed, Jul 18, 2012 at 6:21 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> Well, I personally like the '{}' syntax more than '--group-events or --group-reads
> option in front', it feels more user friendly.. anyway, we can easily have both ways.

I like the actual visual grouping better, too.

Also, it doesn't require us to define what

   -e E1,E2 --group-events -e E3,E4

means.  Does --group-events also apply to the first parameter?


> As for the group attributes and group leader sampling, I don't mind omitting
> them at this point and get back to that if we find it useful in future.

Just define the first event the leader.  What reason is there which
prevents this?


I can only second what Andi wrote: just get it done quickly.  This is
functionality that is desperately needed.

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Jiri Olsa]

On Wed, Jul 18, 2012 at 08:34:10AM -0400, Ulrich Drepper wrote:
> On Wed, Jul 18, 2012 at 6:21 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > Well, I personally like the '{}' syntax more than '--group-events or --group-reads
> > option in front', it feels more user friendly.. anyway, we can easily have both ways.
> 
> I like the actual visual grouping better, too.
> 
> Also, it doesn't require us to define what
> 
>    -e E1,E2 --group-events -e E3,E4
> 
> means.  Does --group-events also apply to the first parameter?
> 
> 
> > As for the group attributes and group leader sampling, I don't mind omitting
> > them at this point and get back to that if we find it useful in future.
> 
> Just define the first event the leader.  What reason is there which
> prevents this?

yep, no problem.. first one is the leader

The group sampling is just another feature where you enable sampling
only for the leader and the rest of the events in the group are only
being read on leader's sample - they don't sample by themselves.

But this feature needs more testing, and event modifier syntax rethinking ;)
I'll send it later..

> I can only second what Andi wrote: just get it done quickly.  This is
> functionality that is desperately needed.

I have new patchset ready with group modifiers adding modifiers
to events in the group, like:

 {cycles:u,instructions:u}:p

adds 'p' to both events in the group
(in current patchset group modifier overwrites events modifiers)

if there's strong opinion/reasons against it, I can simply remove
it, but it looks helpful

jirka

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Stephane Eranian]

On Mon, Jul 2, 2012 at 3:11 PM, Namhyung Kim <namhyung@gmail.com> wrote:
> 2012-07-02 (월), 12:15 +0200, Jiri Olsa:
>> On Mon, Jul 02, 2012 at 10:53:44AM +0900, Namhyung Kim wrote:
>> > Just a question, is there a way to know about the grouping at perf
>> > report time?
>>
>> nope, AFAIK only ID and perf_event_attr is stored for event
>> grouping is known only for record time
>>
>
> I heard that Arnaldo (or Stephane) wanted to make perf report
> group-aware or such so that it can show related events together. But to
> do that, it seems we need to change the data file format first, right?
>
Yes, you'd like something like that. The idea is to use an event to sample
and the others to compute event deltas. For instance, how many loads/stores
instructions retired for every 100k instructions retired. For that you need
grouping. The kernel supports this already, the perf tool does not.

I don't  think you need to change the file format. Everything is there
already in the headers. OF course, you perf report needs to detect
this mode and adjust the output. IF not, then it should only display
samples coming from the first event. Not clear how you'd display
the information in perf report or perf annotate at this point yet.

> Any idea?
>
>
> --
> Regards,
> Namhyung Kim
>
>

Re: [PATCHv3 0/3] perf tool: Add new event group management

[David Ahern]

Hi Jiri:

On 6/29/12 3:08 AM, Jiri Olsa wrote:
> hi,
> adding support for creating event groups based on the way they
> are specified on the command line.
>
> This patchset adds the '{}' style grammar to express event group,
> allowing so far only the 'event modifier' as group modifier.
>
> The sample/leader modifier changes are ready, but I'm getting
> some weird numbers in tests, so I'll send it later. Meanwhile,
> it'd be nice to have at least this patchset in.. ;)
>
>
> Attached patches:
>    1/3 perf, tool: Add support to parse event group syntax
>    2/3 perf, tool: Enable grouping logic for parsed events
>    3/3 perf, test: Add automated tests for event group parsing

Would you mind updating perf-stat and perf-record Documentation to 
include the group option?

David

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Jiri Olsa]

On Sun, Jul 01, 2012 at 08:25:23PM -0600, David Ahern wrote:
> Hi Jiri:
> 
> On 6/29/12 3:08 AM, Jiri Olsa wrote:
> >hi,
> >adding support for creating event groups based on the way they
> >are specified on the command line.
> >
> >This patchset adds the '{}' style grammar to express event group,
> >allowing so far only the 'event modifier' as group modifier.
> >
> >The sample/leader modifier changes are ready, but I'm getting
> >some weird numbers in tests, so I'll send it later. Meanwhile,
> >it'd be nice to have at least this patchset in.. ;)
> >
> >
> >Attached patches:
> >   1/3 perf, tool: Add support to parse event group syntax
> >   2/3 perf, tool: Enable grouping logic for parsed events
> >   3/3 perf, test: Add automated tests for event group parsing
> 
> Would you mind updating perf-stat and perf-record Documentation to
> include the group option?

yes, Arnaldo already stressed that.. ;) I'll send it
with my next patchset

jirka

> 
> David

Re: [PATCHv3 0/3] perf tool: Add new event group management

[Peter Zijlstra]

On Fri, 2012-06-29 at 11:08 +0200, Jiri Olsa wrote:
> hi,
> adding support for creating event groups based on the way they
> are specified on the command line.
> 
> This patchset adds the '{}' style grammar to express event group,
> allowing so far only the 'event modifier' as group modifier.
> 
> The sample/leader modifier changes are ready, but I'm getting
> some weird numbers in tests, so I'll send it later. Meanwhile,
> it'd be nice to have at least this patchset in.. ;)
> 
> 
> Attached patches:
>   1/3 perf, tool: Add support to parse event group syntax
>   2/3 perf, tool: Enable grouping logic for parsed events
>   3/3 perf, test: Add automated tests for event group parsing

Yeah, they look ok, thanks Jiri!