[RFCv2 0/8] perf tool: Add new event group management

[RFCv2 0/8] 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. 

- added 'group=...' syntax to group events (patches 1-5).
  I think this part is quite ready.

- added 'group:1=...' syntax to specify which event to sample (leader)
  and use  PERF_SAMPLE_READ and PERF_FORMAT_GROUP to read all siblings
  on every leader's events (patches 6-8).
  Currently it is only possible to display siblings values by
  'perf report -D' PERF_SAMPLE_READ dump output containing only perf
  internal IDs displayed.. so not very usefull. But I was hoping to
  straighten up the data design/directions before I touch the gui.


Attached patches:
 1/8 perf, tool: Add support to parse event group syntax
 2/8 perf, tool: Enable grouping logic for parsed events
 3/8 perf: Add PERF_EVENT_IOC_ID ioctl to return event ID
 4/8 perf, tool: Use PERF_EVENT_IOC_ID perf ioctl to read event id
 5/8 perf, tool: Separate 'mem:' event scanner bits
 6/8 perf, tool: Add modifier support to group event syntax
 7/8 perf, tool: Add support for parsing PERF_SAMPLE_READ
 8/8 perf, tool: Enable sampling on specified event group leader

thanks for comments,
jirka
---
 include/linux/perf_event.h          |    1 +
 kernel/events/core.c                |    9 +++
 tools/perf/builtin-record.c         |   13 ++---
 tools/perf/builtin-stat.c           |   13 ++---
 tools/perf/builtin-test.c           |   12 ++--
 tools/perf/builtin-top.c            |   12 +---
 tools/perf/util/event.h             |   21 ++++++-
 tools/perf/util/evlist.c            |  120 +++++++++++++++++++++++++++--------
 tools/perf/util/evlist.h            |    8 ++-
 tools/perf/util/evsel.c             |   94 +++++++++++++++++++++-------
 tools/perf/util/evsel.h             |   16 ++---
 tools/perf/util/parse-events-test.c |   93 ++++++++++++++++++++++++++-
 tools/perf/util/parse-events.c      |    6 ++
 tools/perf/util/parse-events.h      |    1 +
 tools/perf/util/parse-events.l      |   39 +++++++++++-
 tools/perf/util/parse-events.y      |   66 +++++++++++++++++---
 tools/perf/util/python.c            |   10 ++-
 tools/perf/util/session.c           |   42 ++++++++++++
 tools/perf/util/session.h           |    5 +-
 19 files changed, 476 insertions(+), 105 deletions(-)

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

[Jiri Olsa]

Adding scanner/parser bits to add group event support into the
event syntax.

The grammar for group is:
  start:    groups
  groups:   groups ';' group
  group:    'group=' events

It is possible to specify group assignement in event syntax like:
   perf record -e group=cycles,faults ls

Creating multiple events within one '-e' option with the ';' separator:
   perf record -e 'group=cycles,faults;group=instructions,cache-misses' ls

The grouping functionality itself is comming in shortly.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/parse-events.c |    5 +++
 tools/perf/util/parse-events.h |    1 +
 tools/perf/util/parse-events.l |    2 +
 tools/perf/util/parse-events.y |   55 ++++++++++++++++++++++++++++++++++-----
 4 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index d4abd00..53c7505 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -791,6 +791,11 @@ int parse_events_add_pmu(struct list_head **list, int *idx,
 	return add_event(list, idx, &attr, name);
 }
 
+int parse_events__group(struct list_head *list __used)
+{
+	return 0;
+}
+
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all)
 {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 8651757..a5e1adf 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -99,6 +99,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);
+int parse_events__group(struct list_head *list);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
 void parse_events_error(struct list_head *list_all,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index cd6614d..c867b34 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -107,6 +107,7 @@ period			{ return term(PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD); }
 branch_type		{ return term(PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE); }
 
 mem:			{ return PE_PREFIX_MEM; }
+group=			{ return PE_PREFIX_GROUP; }
 r{num_raw_hex}		{ return raw(); }
 {num_dec}		{ return value(10); }
 {num_hex}		{ return value(16); }
@@ -118,6 +119,7 @@ r{num_raw_hex}		{ return raw(); }
 -			{ return '-'; }
 ,			{ return ','; }
 :			{ return ':'; }
+;			{ return ';'; }
 =			{ return '='; }
 \|			{ return '|'; }
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 5899a0e..a3898d6 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -27,7 +27,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
@@ -50,6 +50,10 @@ do { \
 %type <head> event_legacy_numeric
 %type <head> event_legacy_raw
 %type <head> event_def
+%type <head> event
+%type <head> events
+%type <head> group
+%type <head> groups
 
 %union
 {
@@ -61,25 +65,62 @@ do { \
 }
 %%
 
+start: groups
+{
+	struct list_head *groups = $1;
+
+	parse_events_update_lists(groups, list_all);
+}
+
+groups:
+groups ';' group
+{
+	struct list_head *group = $3;
+	struct list_head *list  = $1;
+
+	parse_events_update_lists(group, list);
+	$$ = list;
+}
+|
+group
+
+group:
+PE_PREFIX_GROUP events
+{
+	struct list_head *list = $2;
+
+	ABORT_ON(parse_events__group(list));
+	$$ = list;
+}
+|
+events
+
 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 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, list_all);
+	ABORT_ON(parse_events_modifier(list, $2));
+	$$ = list;
 }
 |
 event_def
-{
-	parse_events_update_lists($1, list_all);
-}
 
 event_def: event_pmu |
 	   event_legacy_symbol |
-- 
1.7.7.6

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

[Jiri Olsa]

The current event grouping is basic. If you specify the '--group'
option for record/stat/top command, all the specified events become
members of a single group with the first event as a group leader.

This patch adds a functionality that allows to create event groups
based on the way they are specified on the command line. The 'group='
syntax introduced in earlier patch is used. The current '--group/-g'
option behaviour remains intact.

It is possible to create a group with following:
  perf record -e group=cycles,faults ls

creating single event group containing 'cycles' and 'faults' events,
and with cycles event as a 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 group=cpu-clock,task-clock ls

  # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
  perf record -e "group=cpu-clock,task-clock;group=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

  # 2 groups (cpu-clock,task-clock) (minor-faults,major-faults)
  perf record -v -e group=cpu-clock,task-clock \
   -e group=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

Updated automated test to check on group_leader settings.

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            |   34 +++++++++++++------
 tools/perf/util/evlist.h            |    5 ++-
 tools/perf/util/evsel.c             |   46 ++++++++++++++++----------
 tools/perf/util/evsel.h             |   10 ++---
 tools/perf/util/parse-events-test.c |   61 +++++++++++++++++++++++++++++++++-
 tools/perf/util/parse-events.c      |    3 +-
 tools/perf/util/python.c            |    7 +++-
 11 files changed, 144 insertions(+), 68 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 87fc68d..3e0ef7d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -177,18 +177,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.
@@ -203,16 +203,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 c941bb6..9dffc8f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -282,10 +282,6 @@ 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;
-
-	if (group && evsel != first)
-		group_fd = first->fd;
 
 	if (scale)
 		attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED |
@@ -294,15 +290,13 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 	attr->inherit = !no_inherit;
 
 	if (system_wide)
-		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
-						group, group_fd);
+		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus);
 	if (!target_pid && !target_tid && (!group || evsel == first)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
 
-	return perf_evsel__open_per_thread(evsel, evsel_list->threads,
-					   group, group_fd);
+	return perf_evsel__open_per_thread(evsel, evsel_list->threads);
 }
 
 /*
@@ -459,6 +453,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 7c61eef..c19d2d7 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));
@@ -736,7 +736,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 e3c63ae..38944a2 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -841,17 +841,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;
 
@@ -879,8 +876,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 1986d80..486f939 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -107,6 +107,26 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 	evlist->nr_entries += nr_entries;
 }
 
+void perf_evlist__group_list(struct list_head *list)
+{
+	struct perf_evsel *evsel, *first;
+
+	first = list_entry(list->next, struct perf_evsel, node);
+
+	list_for_each_entry(evsel, list, node)
+		if (evsel != first)
+			evsel->leader = first;
+}
+
+int perf_evlist__group(struct perf_evlist *evlist)
+{
+	if (!evlist->nr_entries)
+		return -EINVAL;
+
+	perf_evlist__group_list(&evlist->entries);
+	return 0;
+}
+
 int perf_evlist__add_default(struct perf_evlist *evlist)
 {
 	struct perf_event_attr attr = {
@@ -740,21 +760,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 21f1c9e..863789a 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -78,7 +78,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);
@@ -122,4 +122,7 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 				   struct list_head *list,
 				   int nr_entries);
 
+void perf_evlist__group_list(struct list_head *list);
+int 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 8c13dbc..445ba60 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -16,7 +16,6 @@
 #include "thread_map.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)
 {
@@ -293,9 +292,24 @@ 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;
+
+	if (!leader)
+		return -1;
+
+	/*
+	 * Leader must be already processed/open,
+	 * if not it's a bug.
+	 */
+	assert(leader->fd);
+
+	return FD(leader, cpu, thread);
+}
+
 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;
@@ -311,13 +325,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],
@@ -327,8 +343,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);
 		}
 	}
 
@@ -372,8 +389,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 */
@@ -383,23 +399,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 3d6b3e4..e749d5c 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -66,6 +66,7 @@ struct perf_evsel {
 		void		*data;
 	} handler;
 	bool 			supported;
+	struct perf_evsel	*leader;
 };
 
 struct cpu_map;
@@ -91,14 +92,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-test.c b/tools/perf/util/parse-events-test.c
index 73ff0af..baa4caf 100644
--- a/tools/perf/util/parse-events-test.c
+++ b/tools/perf/util/parse-events-test.c
@@ -388,8 +388,7 @@ static int test__checkevent_list(struct perf_evlist *evlist)
 
 static int test__checkevent_pmu_name(struct perf_evlist *evlist)
 {
-	struct perf_evsel *evsel = list_entry(evlist->entries.next,
-					      struct perf_evsel, node);
+	struct perf_evsel *evsel;
 
 	/* cpu/config=1,name=krava1/u */
 	evsel = list_entry(evlist->entries.next, struct perf_evsel, node);
@@ -431,6 +430,60 @@ static int test__checkevent_pmu_branch_type(struct perf_evlist *evlist)
 	return 0;
 }
 
+static int test__checkevent_group(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel, *leader;
+
+	TEST_ASSERT_VAL("wrong number of entries", 6 == evlist->nr_entries);
+
+	/* cycles */
+	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_CPU_CYCLES == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	/* faults */
+	evsel = list_entry(evsel->node.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 leader", evsel->leader == leader);
+
+	/* r1 */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config", 1 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	/* mem:0 */
+	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config", 0 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong bp_type", (HW_BREAKPOINT_R | HW_BREAKPOINT_W) ==
+					 evsel->attr.bp_type);
+	TEST_ASSERT_VAL("wrong bp_len", HW_BREAKPOINT_LEN_4 ==
+					evsel->attr.bp_len);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	/* cpu/config=2,period=1000/u */
+	evsel = leader = list_entry(evsel->node.next, struct perf_evsel, node);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",  2 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong sample_period",
+			1000 == evsel->attr.sample_period);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	/* instructions:h" */
+	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 leader", evsel->leader == leader);
+
+	return 0;
+}
+
 static struct test__event_st {
 	const char *name;
 	__u32 type;
@@ -544,6 +597,10 @@ static struct test__event_st {
 		.name  = "cpu/config=1,branch_type=hv|any_ret|1,config2=2/u",
 		.check = test__checkevent_pmu_branch_type,
 	},
+	[27] = {
+		.name  = "group=cycles,faults;r1,mem:0:u;group=cpu/config=2,period=1000/u,instructions:h",
+		.check = test__checkevent_group,
+	},
 };
 
 #define TEST__EVENTS_CNT (sizeof(test__events) / sizeof(struct test__event_st))
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 53c7505..5110916 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -791,8 +791,9 @@ int parse_events_add_pmu(struct list_head **list, int *idx,
 	return add_event(list, idx, &attr, name);
 }
 
-int parse_events__group(struct list_head *list __used)
+int parse_events__group(struct list_head *list)
 {
+	perf_evlist__group_list(list);
 	return 0;
 }
 
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

[PATCH 3/8] perf: Add PERF_EVENT_IOC_ID ioctl to return event ID

[Jiri Olsa]

The only way to get the event ID is by reading the event fd,
followed by parsing the ID value out of the returned data.

While this is ok for current read format used by perf tool,
it is not ok when we use PERF_FORMAT_GROUP format.

With this format the data are returned for the whole group
and there's no way to find out what ID belongs to our fd
(if we are not group leader event).

Adding a simple ioctl that returns event primary ID for given fd.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 include/linux/perf_event.h |    1 +
 kernel/events/core.c       |    9 +++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index ddbb6a9..3173ae2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -284,6 +284,7 @@ struct perf_event_attr {
 #define PERF_EVENT_IOC_PERIOD		_IOW('$', 4, __u64)
 #define PERF_EVENT_IOC_SET_OUTPUT	_IO ('$', 5)
 #define PERF_EVENT_IOC_SET_FILTER	_IOW('$', 6, char *)
+#define PERF_EVENT_IOC_ID		_IOR('$', 7, u64 *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index a6a9ec4..3ef2a5f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3266,6 +3266,15 @@ static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case PERF_EVENT_IOC_PERIOD:
 		return perf_event_period(event, (u64 __user *)arg);
 
+	case PERF_EVENT_IOC_ID:
+	{
+		u64 id = primary_event_id(event);
+
+		if (copy_to_user((void __user *)arg, &id, sizeof(id)))
+			return -EFAULT;
+		return 0;
+	}
+
 	case PERF_EVENT_IOC_SET_OUTPUT:
 	{
 		struct perf_event *output_event = NULL;
-- 
1.7.7.6

[PATCH 4/8] perf, tool: Use PERF_EVENT_IOC_ID perf ioctl to read event id

[Jiri Olsa]

Changing the way we retrieve the event ID. Instead of parsing out
the ID out of the read data, using the PERF_EVENT_IOC_ID ioctl.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/evlist.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 486f939..0ffae44 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -339,19 +339,12 @@ static int perf_evlist__id_add_fd(struct perf_evlist *evlist,
 				  struct perf_evsel *evsel,
 				  int cpu, int thread, int fd)
 {
-	u64 read_data[4] = { 0, };
-	int id_idx = 1; /* The first entry is the counter value */
+	u64 id;
 
-	if (!(evsel->attr.read_format & PERF_FORMAT_ID) ||
-	    read(fd, &read_data, sizeof(read_data)) == -1)
-		return -1;
-
-	if (evsel->attr.read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
-		++id_idx;
-	if (evsel->attr.read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
-		++id_idx;
+	if (ioctl(fd, PERF_EVENT_IOC_ID, &id))
+		return -EFAULT;
 
-	perf_evlist__id_add(evlist, evsel, cpu, thread, read_data[id_idx]);
+	perf_evlist__id_add(evlist, evsel, cpu, thread, id);
 	return 0;
 }
 
-- 
1.7.7.6

[PATCH 5/8] perf, tool: Separate 'mem:' event scanner bits

[Jiri Olsa]

Separating 'mem:' scanner processing, so we can parse out
modifier specifically and dont clash with other rules.

This is just precaution for the future, so we dont need to worry
about the rules clashing where we need to parse out any sub-rule
of global rules.

Learning bison/flex as I go... ;)

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

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5110916..54bd4d0 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -888,6 +888,7 @@ int parse_events(struct perf_evlist *evlist, const char *str, int unset __used)
 
 	parse_events__flush_buffer(buffer);
 	parse_events__delete_buffer(buffer);
+	parse_events_lex_destroy();
 
 	if (!ret) {
 		int entries = idx - evlist->nr_entries;
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index c867b34..aeadbbd 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -1,5 +1,6 @@
 
 %option prefix="parse_events_"
+%option stack
 
 %{
 #include <errno.h>
@@ -50,6 +51,8 @@ static int term(int type)
 
 %}
 
+%x mem
+
 num_dec		[0-9]+
 num_hex		0x[a-fA-F0-9]+
 num_raw_hex	[a-fA-F0-9]+
@@ -106,14 +109,13 @@ name			{ return term(PARSE_EVENTS__TERM_TYPE_NAME); }
 period			{ return term(PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD); }
 branch_type		{ return term(PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE); }
 
-mem:			{ return PE_PREFIX_MEM; }
+mem:			{ BEGIN(mem); return PE_PREFIX_MEM; }
 group=			{ return PE_PREFIX_GROUP; }
 r{num_raw_hex}		{ return raw(); }
 {num_dec}		{ return value(10); }
 {num_hex}		{ return value(16); }
 
 {modifier_event}	{ return str(PE_MODIFIER_EVENT); }
-{modifier_bp}		{ return str(PE_MODIFIER_BP); }
 {name}			{ return str(PE_NAME); }
 "/"			{ return '/'; }
 -			{ return '-'; }
@@ -123,6 +125,26 @@ r{num_raw_hex}		{ return raw(); }
 =			{ return '='; }
 \|			{ return '|'; }
 
+<mem>{
+{modifier_bp}		{ return str(PE_MODIFIER_BP); }
+:			{ return ':'; }
+{num_dec}		{ return value(10); }
+{num_hex}		{ return value(16); }
+	/*
+	 * We need to separate 'mem:' scanner part, in order to get specific
+	 * modifier bits parsed out. Othrewise we would need to handle PE_NAME
+	 * and we'd need to parse it manually. During the escape from <mem>
+	 * state we need to put the escaping char back, so we dont miss it.
+	 * TODO there should be better way..
+	 */
+.			{ unput(*parse_events_text); BEGIN(INITIAL); }
+	/*
+	 * We destroy the scanner after reaching EOF,
+	 * but anyway just to be sure get back to INIT state.
+	 */
+<<EOF>>			{ BEGIN(INITIAL); }
+}
+
 %%
 
 int parse_events_wrap(void)
-- 
1.7.7.6

[PATCH 6/8] perf, tool: Add modifier support to group event syntax

[Jiri Olsa]

Adding a way to specify the group modifier. Currently it consists
by simple number saying which event inside the group will be group
leader and do the sampling.

The new group syntax is:
  start:     groups
  groups:    groups ';' group
  group:     'group=' events | 'group:' group_mod '=' events
  group_mod: number

It is possible to specify group assignement in event syntax like:
  perf record -e group:1=cycles,faults ls

with meaning: the cycles will sample the faults will be read.

This patch adds only syntax parsing bits. The code doing the actual
group processing follows shortly.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/parse-events-test.c |   26 ++++++++++++++++++++++++++
 tools/perf/util/parse-events.c      |    2 +-
 tools/perf/util/parse-events.h      |    2 +-
 tools/perf/util/parse-events.l      |   13 ++++++++++++-
 tools/perf/util/parse-events.y      |   13 +++++++++++--
 5 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/parse-events-test.c b/tools/perf/util/parse-events-test.c
index baa4caf..c2f7bd1 100644
--- a/tools/perf/util/parse-events-test.c
+++ b/tools/perf/util/parse-events-test.c
@@ -484,6 +484,28 @@ static int test__checkevent_group(struct perf_evlist *evlist)
 	return 0;
 }
 
+static int test__checkevent_group_mod(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel, *leader;
+
+	TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
+
+	/* cycles */
+	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_CPU_CYCLES == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
+
+	/* faults */
+	evsel = list_entry(evsel->node.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 leader", evsel->leader == leader);
+
+	return 0;
+}
 static struct test__event_st {
 	const char *name;
 	__u32 type;
@@ -601,6 +623,10 @@ static struct test__event_st {
 		.name  = "group=cycles,faults;r1,mem:0:u;group=cpu/config=2,period=1000/u,instructions:h",
 		.check = test__checkevent_group,
 	},
+	[28] = {
+		.name  = "group:1=cycles,faults",
+		.check = test__checkevent_group_mod,
+	},
 };
 
 #define TEST__EVENTS_CNT (sizeof(test__events) / sizeof(struct test__event_st))
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 54bd4d0..d7812bd 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -791,7 +791,7 @@ int parse_events_add_pmu(struct list_head **list, int *idx,
 	return add_event(list, idx, &attr, name);
 }
 
-int parse_events__group(struct list_head *list)
+int parse_events__group(struct list_head *list, int mod __used)
 {
 	perf_evlist__group_list(list);
 	return 0;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index a5e1adf..90c3bfa 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -99,7 +99,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);
-int parse_events__group(struct list_head *list);
+int parse_events__group(struct list_head *list, int mod);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
 void parse_events_error(struct list_head *list_all,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index aeadbbd..96b9b1b 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -26,6 +26,11 @@ static int value(int base)
 	return __value(parse_events_text, base, PE_VALUE);
 }
 
+static int group_mod(void)
+{
+	return __value(parse_events_text, 10, PE_MODIFIER_GROUP);
+}
+
 static int raw(void)
 {
 	return __value(parse_events_text + 1, 16, PE_RAW);
@@ -51,7 +56,7 @@ static int term(int type)
 
 %}
 
-%x mem
+%x mem group
 
 num_dec		[0-9]+
 num_hex		0x[a-fA-F0-9]+
@@ -110,6 +115,7 @@ period			{ return term(PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD); }
 branch_type		{ return term(PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE); }
 
 mem:			{ BEGIN(mem); return PE_PREFIX_MEM; }
+group:			{ BEGIN(group); return PE_PREFIX_GROUP; }
 group=			{ return PE_PREFIX_GROUP; }
 r{num_raw_hex}		{ return raw(); }
 {num_dec}		{ return value(10); }
@@ -145,6 +151,11 @@ r{num_raw_hex}		{ return raw(); }
 <<EOF>>			{ BEGIN(INITIAL); }
 }
 
+<group>{
+{num_dec}		{ BEGIN(INITIAL); return group_mod(); }
+.			{ return PE_ERROR; }
+}
+
 %%
 
 int parse_events_wrap(void)
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index a3898d6..8586d87 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -25,7 +25,7 @@ do { \
 
 %token PE_VALUE PE_VALUE_SYM PE_RAW PE_TERM
 %token PE_NAME
-%token PE_MODIFIER_EVENT PE_MODIFIER_BP
+%token PE_MODIFIER_EVENT PE_MODIFIER_BP PE_MODIFIER_GROUP
 %token PE_NAME_CACHE_TYPE PE_NAME_CACHE_OP_RESULT
 %token PE_PREFIX_MEM PE_PREFIX_RAW PE_PREFIX_GROUP
 %token PE_ERROR
@@ -33,6 +33,7 @@ do { \
 %type <num> PE_VALUE_SYM
 %type <num> PE_RAW
 %type <num> PE_TERM
+%type <num> PE_MODIFIER_GROUP
 %type <str> PE_NAME
 %type <str> PE_NAME_CACHE_TYPE
 %type <str> PE_NAME_CACHE_OP_RESULT
@@ -85,11 +86,19 @@ groups ';' group
 group
 
 group:
+PE_PREFIX_GROUP PE_MODIFIER_GROUP '=' events
+{
+	struct list_head *list = $4;
+
+	ABORT_ON(parse_events__group(list, $2));
+	$$ = list;
+}
+|
 PE_PREFIX_GROUP events
 {
 	struct list_head *list = $2;
 
-	ABORT_ON(parse_events__group(list));
+	ABORT_ON(parse_events__group(list, 0));
 	$$ = list;
 }
 |
-- 
1.7.7.6

[PATCH 7/8] perf, tool: Add support for parsing PERF_SAMPLE_READ

[Jiri Olsa]

Adding support to parse out the PERF_SAMPLE_READ sample bits.
The code contains both single and group format specification.

this code parse out and prepare prepare PERF_SAMPLE_READ data
into the perf_sample struct. It will be used for group leader
sampling feature comming in shortly.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/builtin-test.c |    4 ++--
 tools/perf/util/event.h   |   21 ++++++++++++++++++++-
 tools/perf/util/evlist.c  |   22 ++++++++++++++++++++++
 tools/perf/util/evlist.h  |    2 ++
 tools/perf/util/evsel.c   |   30 +++++++++++++++++++++++++++---
 tools/perf/util/python.c  |    3 ++-
 tools/perf/util/session.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/session.h |    5 ++++-
 8 files changed, 121 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-test.c b/tools/perf/builtin-test.c
index c19d2d7..7acc689 100644
--- a/tools/perf/builtin-test.c
+++ b/tools/perf/builtin-test.c
@@ -564,7 +564,7 @@ static int test__basic_mmap(void)
 		}
 
 		err = perf_event__parse_sample(event, attr.sample_type, sample_size,
-					       false, &sample, false);
+					       false, 0, &sample, false);
 		if (err) {
 			pr_err("Can't parse sample, err = %d\n", err);
 			goto out_munmap;
@@ -786,7 +786,7 @@ static int test__PERF_RECORD(void)
 					nr_events[type]++;
 
 				err = perf_event__parse_sample(event, sample_type,
-							       sample_size, true,
+							       sample_size, true, 0,
 							       &sample, false);
 				if (err < 0) {
 					if (verbose)
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index 1b19728..221266a 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -69,6 +69,23 @@ struct sample_event {
 	u64 array[];
 };
 
+struct sample_read_value {
+	u64 value;
+	u64 id;
+};
+
+struct sample_read {
+	u64 time_enabled;
+	u64 time_running;
+	union {
+		struct {
+			u64 nr;
+			struct sample_read_value *values;
+		} group;
+		struct sample_read_value one;
+	};
+};
+
 struct perf_sample {
 	u64 ip;
 	u32 pid, tid;
@@ -82,6 +99,7 @@ struct perf_sample {
 	void *raw_data;
 	struct ip_callchain *callchain;
 	struct branch_stack *branch_stack;
+	struct sample_read read;
 };
 
 #define BUILD_ID_SIZE 20
@@ -199,7 +217,8 @@ const char *perf_event__name(unsigned int id);
 
 int perf_event__parse_sample(const union perf_event *event, u64 type,
 			     int sample_size, bool sample_id_all,
-			     struct perf_sample *sample, bool swapped);
+			     u64 read_format, struct perf_sample *sample,
+			     bool swapped);
 int perf_event__synthesize_sample(union perf_event *event, u64 type,
 				  const struct perf_sample *sample,
 				  bool swapped);
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 0ffae44..bd48bb3 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -693,6 +693,28 @@ u64 perf_evlist__sample_type(const struct perf_evlist *evlist)
 	return first->attr.sample_type;
 }
 
+bool perf_evlist__valid_read_format(const struct perf_evlist *evlist)
+{
+	struct perf_evsel *pos, *first;
+
+	pos = first = list_entry(evlist->entries.next, struct perf_evsel, node);
+
+	list_for_each_entry_continue(pos, &evlist->entries, node) {
+		if (first->attr.read_format != pos->attr.read_format)
+			return false;
+	}
+
+	return true;
+}
+
+u64 perf_evlist__read_format(const struct perf_evlist *evlist)
+{
+	struct perf_evsel *first;
+
+	first = list_entry(evlist->entries.next, struct perf_evsel, node);
+	return first->attr.read_format;
+}
+
 u16 perf_evlist__id_hdr_size(const struct perf_evlist *evlist)
 {
 	struct perf_evsel *first;
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 863789a..c2fe77b 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -111,12 +111,14 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, const char *target_pid,
 void perf_evlist__delete_maps(struct perf_evlist *evlist);
 int perf_evlist__set_filters(struct perf_evlist *evlist);
 
+u64 perf_evlist__read_format(const const struct perf_evlist *evlist);
 u64 perf_evlist__sample_type(const struct perf_evlist *evlist);
 bool perf_evlist__sample_id_all(const const struct perf_evlist *evlist);
 u16 perf_evlist__id_hdr_size(const struct perf_evlist *evlist);
 
 bool perf_evlist__valid_sample_type(const struct perf_evlist *evlist);
 bool perf_evlist__valid_sample_id_all(const struct perf_evlist *evlist);
+bool perf_evlist__valid_read_format(const struct perf_evlist *evlist);
 
 void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 				   struct list_head *list,
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 445ba60..42f5256 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -465,7 +465,8 @@ static bool sample_overlap(const union perf_event *event,
 
 int perf_event__parse_sample(const union perf_event *event, u64 type,
 			     int sample_size, bool sample_id_all,
-			     struct perf_sample *data, bool swapped)
+			     u64 read_format, struct perf_sample *data,
+			     bool swapped)
 {
 	const u64 *array;
 
@@ -554,8 +555,31 @@ int perf_event__parse_sample(const union perf_event *event, u64 type,
 	}
 
 	if (type & PERF_SAMPLE_READ) {
-		fprintf(stderr, "PERF_SAMPLE_READ is unsupported for now\n");
-		return -1;
+		if (read_format & PERF_FORMAT_GROUP)
+			data->read.group.nr = *array;
+		else
+			data->read.one.value = *array;
+
+		array++;
+
+		if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {
+			data->read.time_enabled = *array;
+			array++;
+		}
+
+		if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) {
+			data->read.time_running = *array;
+			array++;
+		}
+
+		if (read_format & PERF_FORMAT_GROUP) {
+			data->read.group.values = (struct sample_read_value *) array;
+			array = (void *) array + data->read.group.nr *
+				sizeof(struct sample_read_value);
+		} else {
+			data->read.one.id = *array;
+			array++;
+		}
 	}
 
 	if (type & PERF_SAMPLE_CALLCHAIN) {
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 419c29e..477f9a8 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -807,7 +807,8 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
 		first = list_entry(evlist->entries.next, struct perf_evsel, node);
 		err = perf_event__parse_sample(event, first->attr.sample_type,
 					       perf_evsel__sample_size(first),
-					       sample_id_all, &pevent->sample, false);
+					       sample_id_all, first->attr.read_format,
+					       &pevent->sample, false);
 		if (err)
 			return PyErr_Format(PyExc_OSError,
 					    "perf: can't parse sample, err=%d", err);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 9412e3b..b64f017 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -70,6 +70,11 @@ static int perf_session__open(struct perf_session *self, bool force)
 		goto out_close;
 	}
 
+	if (!perf_evlist__valid_read_format(self->evlist)) {
+		pr_err("non matching read_format");
+		goto out_close;
+	}
+
 	self->size = input_stat.st_size;
 	return 0;
 
@@ -86,6 +91,7 @@ void perf_session__update_sample_type(struct perf_session *self)
 	self->sample_id_all = perf_evlist__sample_id_all(self->evlist);
 	self->id_hdr_size = perf_evlist__id_hdr_size(self->evlist);
 	self->host_machine.id_hdr_size = self->id_hdr_size;
+	self->read_format = perf_evlist__read_format(self->evlist);
 }
 
 int perf_session__create_kernel_maps(struct perf_session *self)
@@ -785,6 +791,39 @@ static void perf_session__print_tstamp(struct perf_session *session,
 		printf("%" PRIu64 " ", sample->time);
 }
 
+static void sample_read__printf(struct perf_session *session,
+				struct perf_sample *sample)
+{
+	u64 read_format = session->read_format;
+
+	printf("... sample_read:\n");
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
+		printf("...... time enabled %016" PRIx64 "\n",
+		       sample->read.time_enabled);
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
+		printf("...... time running %016" PRIx64 "\n",
+		       sample->read.time_running);
+
+	if (read_format & PERF_FORMAT_GROUP) {
+		u64 i;
+
+		printf(".... group nr %" PRIu64 "\n", sample->read.group.nr);
+
+		for (i = 0; i < sample->read.group.nr; i++) {
+			struct sample_read_value *value;
+
+			value = &sample->read.group.values[i];
+			printf("..... id %016" PRIx64
+			       ", value %016" PRIx64 "\n",
+			       value->id, value->value);
+		}
+	} else
+		printf("..... id %016" PRIx64 ", value %016" PRIx64 "\n",
+			sample->read.one.id, sample->read.one.value);
+}
+
 static void dump_event(struct perf_session *session, union perf_event *event,
 		       u64 file_offset, struct perf_sample *sample)
 {
@@ -818,6 +857,9 @@ static void dump_sample(struct perf_session *session, union perf_event *event,
 
 	if (session->sample_type & PERF_SAMPLE_BRANCH_STACK)
 		branch_stack__printf(sample);
+
+	if (session->sample_type & PERF_SAMPLE_READ)
+		sample_read__printf(session, sample);
 }
 
 static struct machine *
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 7a5434c..8d2ea3c 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -42,6 +42,7 @@ struct perf_session {
 	struct hists		hists;
 	u64			sample_type;
 	int			sample_size;
+	u64			read_format;
 	int			fd;
 	bool			fd_pipe;
 	bool			repipe;
@@ -134,7 +135,9 @@ static inline int perf_session__parse_sample(struct perf_session *session,
 {
 	return perf_event__parse_sample(event, session->sample_type,
 					session->sample_size,
-					session->sample_id_all, sample,
+					session->sample_id_all,
+					session->read_format,
+					sample,
 					session->header.needs_swap);
 }
 
-- 
1.7.7.6

[PATCH 8/8] perf, tool: Enable sampling on specified event group leader

[Jiri Olsa]

Adding the functionality to 'group:modifier=...' event syntax.
Allowing user to select an event out of the defined group by
event index ( = event order placement in the group).

This selected event becomes group leader and will be the
only one which be sampling.

The rest of the events counts are being read on each leader
sample by PERF_SAMPLE_READ sample type.

Following example:
  perf record -e group:1=cycles,faults 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.

It is possible to display the data by 'perf report -D' command.
For each sample the PERF_SAMPLE_READ data are displayed if it's
included. More sophisticated output comming shortly.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/evlist.c            |   63 ++++++++++++++++++++++++++++------
 tools/perf/util/evlist.h            |    3 +-
 tools/perf/util/evsel.c             |   18 +++++++++-
 tools/perf/util/evsel.h             |    6 ++--
 tools/perf/util/parse-events-test.c |   20 +++++++----
 tools/perf/util/parse-events.c      |    5 +--
 6 files changed, 88 insertions(+), 27 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index bd48bb3..b0d5038 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -48,18 +48,27 @@ struct perf_evlist *perf_evlist__new(struct cpu_map *cpus,
 	return evlist;
 }
 
+static void update_leader_sample(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel;
+
+	list_for_each_entry(evsel, &evlist->entries, node)
+		if (evsel->leader_sample)
+			evlist->leader_sample = true;
+}
+
 void perf_evlist__config_attrs(struct perf_evlist *evlist,
 			       struct perf_record_opts *opts)
 {
-	struct perf_evsel *evsel, *first;
+	struct perf_evsel *evsel;
+
+	update_leader_sample(evlist);
 
 	if (evlist->cpus->map[0] < 0)
 		opts->no_inherit = true;
 
-	first = list_entry(evlist->entries.next, struct perf_evsel, node);
-
 	list_for_each_entry(evsel, &evlist->entries, node) {
-		perf_evsel__config(evsel, opts, first);
+		perf_evsel__config(evlist, evsel, opts);
 
 		if (evlist->nr_entries > 1)
 			evsel->attr.sample_type |= PERF_SAMPLE_ID;
@@ -107,15 +116,48 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 	evlist->nr_entries += nr_entries;
 }
 
-void perf_evlist__group_list(struct list_head *list)
+static struct perf_evsel* get_evsel_nth_list(struct list_head *list, int n)
 {
-	struct perf_evsel *evsel, *first;
+	struct perf_evsel *evsel;
+	int i = 1;
 
-	first = list_entry(list->next, struct perf_evsel, node);
+	list_for_each_entry(evsel, list, node)
+		if (i++ == n)
+			return evsel;
+
+	return NULL;
+}
+
+int perf_evlist__group_list(struct list_head *list, int leader_sample)
+{
+	struct perf_evsel *evsel, *leader;
+
+	if (leader_sample) {
+		leader = get_evsel_nth_list(list, leader_sample);
+		if (!leader) {
+			fprintf(stderr,
+				"failed: leader number out of bounds\n");
+			return -EINVAL;
+		}
+
+		leader->leader_sample = true;
+
+		/*
+		 * Make the leader first in the list, because we need it
+		 * to be opened first, so we can use its fd for group fd.
+		 */
+		if (leader_sample != 1) {
+			list_del(&leader->node);
+			list_add(&leader->node, list);
+		}
+	} else
+		leader = list_entry(list->next, struct perf_evsel, node);
 
 	list_for_each_entry(evsel, list, node)
-		if (evsel != first)
-			evsel->leader = first;
+		if (evsel != leader)
+			evsel->leader = leader;
+
+	return 0;
 }
 
 int perf_evlist__group(struct perf_evlist *evlist)
@@ -123,8 +165,7 @@ int perf_evlist__group(struct perf_evlist *evlist)
 	if (!evlist->nr_entries)
 		return -EINVAL;
 
-	perf_evlist__group_list(&evlist->entries);
-	return 0;
+	return perf_evlist__group_list(&evlist->entries, 0);
 }
 
 int perf_evlist__add_default(struct perf_evlist *evlist)
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index c2fe77b..6fb380c 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -28,6 +28,7 @@ struct perf_evlist {
 		pid_t	pid;
 	} workload;
 	bool		 overwrite;
+	bool		 leader_sample;
 	union perf_event event_copy;
 	struct perf_mmap *mmap;
 	struct pollfd	 *pollfd;
@@ -124,7 +125,7 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 				   struct list_head *list,
 				   int nr_entries);
 
-void perf_evlist__group_list(struct list_head *list);
+int perf_evlist__group_list(struct list_head *list, int leader_sample);
 int 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 42f5256..0e0756a 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -62,18 +62,27 @@ struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr, int idx)
 	return evsel;
 }
 
-void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
-			struct perf_evsel *first)
+void perf_evsel__config(struct perf_evlist *evlist, struct perf_evsel *evsel,
+			struct perf_record_opts *opts)
 {
+	struct perf_evsel *first, *leader = evsel->leader;
 	struct perf_event_attr *attr = &evsel->attr;
 	int track = !evsel->idx; /* only the first counter needs these */
 
+	first = list_entry(evlist->entries.next, struct perf_evsel, node);
+
 	attr->sample_id_all = opts->sample_id_all_missing ? 0 : 1;
 	attr->inherit	    = !opts->no_inherit;
 	attr->read_format   = PERF_FORMAT_TOTAL_TIME_ENABLED |
 			      PERF_FORMAT_TOTAL_TIME_RUNNING |
 			      PERF_FORMAT_ID;
 
+	if (evlist->leader_sample) {
+		attr->read_format |= PERF_FORMAT_GROUP;
+		attr->sample_type |= PERF_SAMPLE_READ;
+		attr->inherit = 0;
+	}
+
 	attr->sample_type  |= PERF_SAMPLE_IP | PERF_SAMPLE_TID;
 
 	/*
@@ -91,6 +100,11 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
 		}
 	}
 
+	if (leader && leader->leader_sample) {
+		attr->sample_freq   = 0;
+		attr->sample_period = 0;
+	}
+
 	if (opts->no_samples)
 		attr->sample_freq = 0;
 
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index e749d5c..e40c5ff 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -67,6 +67,7 @@ struct perf_evsel {
 	} handler;
 	bool 			supported;
 	struct perf_evsel	*leader;
+	bool			leader_sample;
 };
 
 struct cpu_map;
@@ -80,9 +81,8 @@ void perf_evsel__init(struct perf_evsel *evsel,
 void perf_evsel__exit(struct perf_evsel *evsel);
 void perf_evsel__delete(struct perf_evsel *evsel);
 
-void perf_evsel__config(struct perf_evsel *evsel,
-			struct perf_record_opts *opts,
-			struct perf_evsel *first);
+void perf_evsel__config(struct perf_evlist *evlist, struct perf_evsel *evsel,
+			struct perf_record_opts *opts);
 
 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);
diff --git a/tools/perf/util/parse-events-test.c b/tools/perf/util/parse-events-test.c
index c2f7bd1..15a8c4d 100644
--- a/tools/perf/util/parse-events-test.c
+++ b/tools/perf/util/parse-events-test.c
@@ -490,22 +490,28 @@ static int test__checkevent_group_mod(struct perf_evlist *evlist)
 
 	TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
 
-	/* cycles */
+	/*
+	 * With modifier :2 we have 'faults leading the list
+	 * and being the leader
+	 */
+
+	/* faults */
 	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 type", PERF_TYPE_SOFTWARE == evsel->attr.type);
 	TEST_ASSERT_VAL("wrong config",
-			PERF_COUNT_HW_CPU_CYCLES == evsel->attr.config);
+			PERF_COUNT_SW_PAGE_FAULTS == evsel->attr.config);
 	TEST_ASSERT_VAL("wrong leader", evsel->leader == NULL);
 
-	/* faults */
+	/* cycles */
 	evsel = list_entry(evsel->node.next, struct perf_evsel, node);
-	TEST_ASSERT_VAL("wrong type", PERF_TYPE_SOFTWARE == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->attr.type);
 	TEST_ASSERT_VAL("wrong config",
-			PERF_COUNT_SW_PAGE_FAULTS == evsel->attr.config);
+			PERF_COUNT_HW_CPU_CYCLES == evsel->attr.config);
 	TEST_ASSERT_VAL("wrong leader", evsel->leader == leader);
 
 	return 0;
 }
+
 static struct test__event_st {
 	const char *name;
 	__u32 type;
@@ -624,7 +630,7 @@ static struct test__event_st {
 		.check = test__checkevent_group,
 	},
 	[28] = {
-		.name  = "group:1=cycles,faults",
+		.name  = "group:2=cycles,faults",
 		.check = test__checkevent_group_mod,
 	},
 };
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index d7812bd..afec4d0 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -791,10 +791,9 @@ int parse_events_add_pmu(struct list_head **list, int *idx,
 	return add_event(list, idx, &attr, name);
 }
 
-int parse_events__group(struct list_head *list, int mod __used)
+int parse_events__group(struct list_head *list, int leader_sample)
 {
-	perf_evlist__group_list(list);
-	return 0;
+	return perf_evlist__group_list(list, leader_sample);
 }
 
 void parse_events_update_lists(struct list_head *list_event,
-- 
1.7.7.6

Re: [RFCv2 0/8] perf tool: Add new event group management

[Jiri Olsa]

oops, forgot to add that this patchset is based on another
one I sent just recently..

http://marc.info/?l=linux-kernel&m=133357111816870&w=2

jirka


On Wed, Apr 04, 2012 at 11:16:08PM +0200, Jiri Olsa wrote:
> hi,
> adding support for creating event groups based on the way they
> are specified on the command line. 
> 
> - added 'group=...' syntax to group events (patches 1-5).
>   I think this part is quite ready.
> 
> - added 'group:1=...' syntax to specify which event to sample (leader)
>   and use  PERF_SAMPLE_READ and PERF_FORMAT_GROUP to read all siblings
>   on every leader's events (patches 6-8).
>   Currently it is only possible to display siblings values by
>   'perf report -D' PERF_SAMPLE_READ dump output containing only perf
>   internal IDs displayed.. so not very usefull. But I was hoping to
>   straighten up the data design/directions before I touch the gui.
> 
> 
> Attached patches:
>  1/8 perf, tool: Add support to parse event group syntax
>  2/8 perf, tool: Enable grouping logic for parsed events
>  3/8 perf: Add PERF_EVENT_IOC_ID ioctl to return event ID
>  4/8 perf, tool: Use PERF_EVENT_IOC_ID perf ioctl to read event id
>  5/8 perf, tool: Separate 'mem:' event scanner bits
>  6/8 perf, tool: Add modifier support to group event syntax
>  7/8 perf, tool: Add support for parsing PERF_SAMPLE_READ
>  8/8 perf, tool: Enable sampling on specified event group leader
> 
> thanks for comments,
> jirka
> ---
>  include/linux/perf_event.h          |    1 +
>  kernel/events/core.c                |    9 +++
>  tools/perf/builtin-record.c         |   13 ++---
>  tools/perf/builtin-stat.c           |   13 ++---
>  tools/perf/builtin-test.c           |   12 ++--
>  tools/perf/builtin-top.c            |   12 +---
>  tools/perf/util/event.h             |   21 ++++++-
>  tools/perf/util/evlist.c            |  120 +++++++++++++++++++++++++++--------
>  tools/perf/util/evlist.h            |    8 ++-
>  tools/perf/util/evsel.c             |   94 +++++++++++++++++++++-------
>  tools/perf/util/evsel.h             |   16 ++---
>  tools/perf/util/parse-events-test.c |   93 ++++++++++++++++++++++++++-
>  tools/perf/util/parse-events.c      |    6 ++
>  tools/perf/util/parse-events.h      |    1 +
>  tools/perf/util/parse-events.l      |   39 +++++++++++-
>  tools/perf/util/parse-events.y      |   66 +++++++++++++++++---
>  tools/perf/util/python.c            |   10 ++-
>  tools/perf/util/session.c           |   42 ++++++++++++
>  tools/perf/util/session.h           |    5 +-
>  19 files changed, 476 insertions(+), 105 deletions(-)

Re: [PATCH 5/8] perf, tool: Separate 'mem:' event scanner bits

[Robert Richter]

On 04.04.12 23:16:13, Jiri Olsa wrote:
> Separating 'mem:' scanner processing, so we can parse out
> modifier specifically and dont clash with other rules.
> 
> This is just precaution for the future, so we dont need to worry
> about the rules clashing where we need to parse out any sub-rule
> of global rules.
> 
> Learning bison/flex as I go... ;)

All this lex/yacc thing is over-engineered. Having this for just
parsing events events is overkill and introduces more problems than it
solves.

I am looking at this because I want to extend perf tools to have
support for dynamic allocated pmus, esp. for IBS. I have had to
completly rework my code because of this change and it is still hard
to find a solution for this. I guess there was a reason to use
lex/yacc, but I don't think we need it for parsing *all* types of
events.

First, not every developer knows about lex and yacc, so this raises
the bar to modify and improve the code. I already had worked with
lex/yacc some years ago. What I remember and learned from it is to
avoid using it because there are easier ways to solve the same
problems. Esp. I learned to keep things simple and not to invent
complex syntax or languages. Unfortunatlly, this is what happens here
(remember, we just want to parse events, nothing more).

It also adds a more complex tool chain and the result of the generated
code is heavily affected by them and the used distro. The discussion
about having precompiled code in the repository had shown its
problems. There is also no control of the quality of the generated
code. With something like the following you are actually stucked to
build perf:

 $ make
 ...
     CC util/parse-events-flex.o
 cc1: warnings being treated as errors
 <stdout>: In function 'yy_get_next_buffer':
 <stdout>:1504:3: error: comparison between signed and unsigned integer expressions
 util/parse-events.l: In function 'parse_events_lex':
 util/parse-events.l:122:1: error: ignoring return value of 'fwrite', declared with attribute warn_unused_result
 make: *** [util/parse-events-flex.o] Error 1

But the main problem I see are side effects of the syntax for one
event type to another. The lexer generates a token stream and the
syntax of each token is predefined, e.g. the word 'config' always
returns the PE_TERM token. Thus, you are not able to use that word for
other purposes, let's say in a tracepoint. Another example is the word
'race' that is always detected as a raw-event token. This side effects
are not easy to discover and to test.

This makes it also hard to extend the syntax. Since certain token
patterns are already defined for a special event type, the same
pattern can not be used again. If the setup of this event fails the
parser is not able to setup a different event type. See this example:

 <pmu>:<some_string>:<modifier>
 <pmu>:<raw_config>:<modifier>

This syntax is already used for a single event type (tracepoints) and
can't be reused anymore. There are options to solve this by defining
the syntax different:

 <pmu>::<some_string>::<modifier>
 <pmu>/<some_string>/:<modifier>

... or so. Alternativly one could write a more complex lexer that goes
back and forth in the buffer using yyless() or so.

One could also argument here that the syntax needs to be well defined,
which would prevent the problems above, but I don't want to implement
a nice syntax, I want to parse events.

The previous implementation worked simple and straightforward by
passing the event string to the event parsers until there was a
match. I haven't had the feeling of unresolvable issues with the plain
C implementation. So why lex/yacc?

But maybe it's already too late for this discussion. Hmm...

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 5/8] perf, tool: Separate 'mem:' event scanner bits

[Jiri Olsa]

On Wed, Apr 11, 2012 at 03:28:18PM +0200, Robert Richter wrote:
> On 04.04.12 23:16:13, Jiri Olsa wrote:
> > Separating 'mem:' scanner processing, so we can parse out
> > modifier specifically and dont clash with other rules.
> > 
> > This is just precaution for the future, so we dont need to worry
> > about the rules clashing where we need to parse out any sub-rule
> > of global rules.
> > 
> > Learning bison/flex as I go... ;)
> 
> All this lex/yacc thing is over-engineered. Having this for just
> parsing events events is overkill and introduces more problems than it
> solves.
> 
> I am looking at this because I want to extend perf tools to have
> support for dynamic allocated pmus, esp. for IBS. I have had to
> completly rework my code because of this change and it is still hard
> to find a solution for this. I guess there was a reason to use
> lex/yacc, but I don't think we need it for parsing *all* types of
> events.
hi,
I could try to help you, please feel free to send me details

> 
> First, not every developer knows about lex and yacc, so this raises
> the bar to modify and improve the code. I already had worked with
> lex/yacc some years ago. What I remember and learned from it is to
> avoid using it because there are easier ways to solve the same
> problems. Esp. I learned to keep things simple and not to invent
> complex syntax or languages. Unfortunatlly, this is what happens here
> (remember, we just want to parse events, nothing more).
> 
> It also adds a more complex tool chain and the result of the generated
> code is heavily affected by them and the used distro. The discussion
> about having precompiled code in the repository had shown its
> problems. There is also no control of the quality of the generated
> code. With something like the following you are actually stucked to

We saw some problems with some RHEL6 using buggy bison version.. but
that's about it AFAIK.

> build perf:
> 
>  $ make
>  ...
>      CC util/parse-events-flex.o
>  cc1: warnings being treated as errors
>  <stdout>: In function 'yy_get_next_buffer':
>  <stdout>:1504:3: error: comparison between signed and unsigned integer expressions
>  util/parse-events.l: In function 'parse_events_lex':
>  util/parse-events.l:122:1: error: ignoring return value of 'fwrite', declared with attribute warn_unused_result
>  make: *** [util/parse-events-flex.o] Error 1

We had to disable some of the warnings to get it compiled.
Above failure is from your modification, right?
Since I dont see mentioned parse-events.l:122 causing the issue.


> 
> But the main problem I see are side effects of the syntax for one
> event type to another. The lexer generates a token stream and the
> syntax of each token is predefined, e.g. the word 'config' always
> returns the PE_TERM token. Thus, you are not able to use that word for
> other purposes, let's say in a tracepoint. Another example is the word
> 'race' that is always detected as a raw-event token. This side effects
> are not easy to discover and to test.

This patch is actually part of the solution for this.
You can add 'start' conditions' to lexer and do specific matching
for specific prefix.

As for the PE_TERM issue you mentioned, the util/parse-events.l file has
following comment addressing this issue:

        /*
         * These are event config hardcoded term names to be specified
         * within xxx/.../ syntax. So far we dont clash with other * names,
         * so we can put them here directly. In case the we have a * conflict
         * in future, this needs to go into '//' condition block.
         */

> 
> This makes it also hard to extend the syntax. Since certain token
> patterns are already defined for a special event type, the same
> pattern can not be used again. If the setup of this event fails the
> parser is not able to setup a different event type. See this example:
> 
>  <pmu>:<some_string>:<modifier>
>  <pmu>:<raw_config>:<modifier>
> 
> This syntax is already used for a single event type (tracepoints) and
> can't be reused anymore. There are options to solve this by defining
> the syntax different:
> 
>  <pmu>::<some_string>::<modifier>
>  <pmu>/<some_string>/:<modifier>
> 
> ... or so. Alternativly one could write a more complex lexer that goes
> back and forth in the buffer using yyless() or so.
> 
> One could also argument here that the syntax needs to be well defined,
> which would prevent the problems above, but I don't want to implement
> a nice syntax, I want to parse events.
> 
> The previous implementation worked simple and straightforward by
> passing the event string to the event parsers until there was a
> match. I haven't had the feeling of unresolvable issues with the plain
> C implementation. So why lex/yacc?

I believe the original reason was to handle event grouping within
the event syntax, which would not be that straightforward with
the old implementation.

It was discussed somewhere in here:
http://marc.info/?t=130105133700002&r=1&w=2
http://marc.info/?t=129065042800002&r=1&w=2
http://marc.info/?l=linux-kernel&m=129067262931833&w=2

My opinion is that using the parser for this made the code more
readable/straightforward, since we dont need to deal with the
actual parsing. But as you said some bison/flex knowledge is
needed to get orientated.

wbr,
jirka

Re: [PATCH 5/8] perf, tool: Separate 'mem:' event scanner bits

[Robert Richter]

On 11.04.12 16:33:13, Jiri Olsa wrote:
> I could try to help you, please feel free to send me details

Jiri, I think I found a solution in between. Will cc you when sending
the patches.

Thanks for your answer and also the pointer to the ml thread, some of
this I was not aware.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [RFCv2 0/8] perf tool: Add new event group management

[Peter Zijlstra]

On Wed, 2012-04-04 at 23:16 +0200, Jiri Olsa wrote:
> - added 'group=...' syntax to group events (patches 1-5).

> - added 'group:1=...' syntax to specify which event to sample (leader) 

I'm still sad about not actually using {} for grouping. Esp. since you
use ';' which needs escaping anyway:

> perf record -e 'group=cycles,faults;group=instructions,cache-misses' ls

Also, how do you name the groups?

Re: [RFCv2 0/8] perf tool: Add new event group management

[Jiri Olsa]

On Sun, Apr 15, 2012 at 05:16:55PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-04-04 at 23:16 +0200, Jiri Olsa wrote:
> > - added 'group=...' syntax to group events (patches 1-5).
> 
> > - added 'group:1=...' syntax to specify which event to sample (leader) 
> 
> I'm still sad about not actually using {} for grouping. Esp. since you
> use ';' which needs escaping anyway:

yea, I realized we need ending separator after all ;)
so right we could use {} then..

so smth like:

  '{cycles,faults}'
   - anonymous group

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

  'group1:1{cycles,faults}
   - group with name 'group1' and modifier '1'

  'group1{cycles,faults},{instructions,cache-misses},faults'
  '{cycles,faults},group1:2{instructions,cache-misses},faults' ls
   - group/event combinations 


group:        group_spec '{' events '}'
group_spec:   name  ':' mod | empty


also, any thoughts about the displaying question? ;)

> I'm looking on how to present this data in perf and it seems we need
> to reset all siblings once we read/store them (in kernel) to the
> leader sample.
> 
> My current thinking is to store siblings' sum values for each
> hists entry of the sample (perf report count unit) .. and display
> them in similar way we display callchains: for each hists entry
> display the sum value for each sibling.
> 
> Could you provide more of your world examples? Your expectations about
> presenting this..
> 
> Maybe we want to make the reset optional, and do some do some other
> math with siblings' values..?

Not sure it could affect the syntax, but it'd be better to have
whole picture

thanks,
jirka

Re: [RFCv2 0/8] perf tool: Add new event group management

[Peter Zijlstra]

On Mon, 2012-04-16 at 14:16 +0200, Jiri Olsa wrote:
> also, any thoughts about the displaying question? ;)
> 
> > I'm looking on how to present this data in perf and it seems we need
> > to reset all siblings once we read/store them (in kernel) to the
> > leader sample.
> > 
> > My current thinking is to store siblings' sum values for each
> > hists entry of the sample (perf report count unit) .. and display
> > them in similar way we display callchains: for each hists entry
> > display the sum value for each sibling.
> > 
> > Could you provide more of your world examples? Your expectations about
> > presenting this..
> > 
> > Maybe we want to make the reset optional, and do some do some other
> > math with siblings' values..?


My thought was to do the exact same thing with it that we do with the
regular multi-event thing (which could be improved too). Simply create
individual event views with variable period based on the sample delta
for that particular event.

So suppose you have a group of 3 with only the leader sampling and you
get tuples like:

 IP=x {1000,  500,  750}
 IP=y {2000,  800, 1500}
 IP=z {3000, 1100, 2000}

This could be decomposed into 3 event views like:

 IP=x,period=1000	IP=x,period=500		IP=x,period=750
 IP=y,period=1000	IP=y,period=300		IP=y,period=750
 IP=z,period=1000	IP=z,period=300		IP=z,period=500

Just like what would've happened if you'd used multiple events like:

 attr = {
	.sample_type = PERF_SAMPLE_IP|PERF_SAMPLE_PERIOD,
	.freq = 1,
	.sample_period = 1000,
 }

Re: [RFCv2 0/8] perf tool: Add new event group management

[Peter Zijlstra]

On Mon, 2012-04-16 at 14:16 +0200, Jiri Olsa wrote:
> group:        group_spec '{' events '}'
> group_spec:   name  ':' mod | empty 

How about something like:

groups: groups ',' group | group
group:	group_name '{' events '}' group_mod
group_name: name ':' | empty
group_mod: ':' group_mods | empty
group_mods: [l]

Re: [RFCv2 0/8] perf tool: Add new event group management

[Jiri Olsa]

On Mon, Apr 16, 2012 at 05:26:02PM +0200, Peter Zijlstra wrote:
> On Mon, 2012-04-16 at 14:16 +0200, Jiri Olsa wrote:
> > group:        group_spec '{' events '}'
> > group_spec:   name  ':' mod | empty 
> 
> How about something like:
> 
> groups: groups ',' group | group
> group:	group_name '{' events '}' group_mod
> group_name: name ':' | empty
> group_mod: ':' group_mods | empty
> group_mods: [l]
> 
> 

so you'd like something like:
  'group1:{cycles,faults}:1

hm, do you want the ':' behind the name? It does not seem necessary,
but might be readable though.. ok :)
  'group1{cycles,faults}:1

thanks,
jirka

Re: [RFCv2 0/8] perf tool: Add new event group management

[Namhyung Kim]

Hi,

On Mon, 16 Apr 2012 17:37:39 +0200, Jiri Olsa wrote:
> On Mon, Apr 16, 2012 at 05:26:02PM +0200, Peter Zijlstra wrote:
>> On Mon, 2012-04-16 at 14:16 +0200, Jiri Olsa wrote:
>> > group:        group_spec '{' events '}'
>> > group_spec:   name  ':' mod | empty 
>> 
>> How about something like:
>> 
>> groups: groups ',' group | group
>> group:	group_name '{' events '}' group_mod
>> group_name: name ':' | empty
>> group_mod: ':' group_mods | empty
>> group_mods: [l]
>> 
>> 
>
> so you'd like something like:
>   'group1:{cycles,faults}:1
>
> hm, do you want the ':' behind the name? It does not seem necessary,
> but might be readable though.. ok :)
>   'group1{cycles,faults}:1
>

+1 for the last form :)

Thanks,
Namhyung

Re: [RFCv2 0/8] perf tool: Add new event group management

[Namhyung Kim]

Hi,

On Mon, 16 Apr 2012 17:26:02 +0200, Peter Zijlstra wrote:
> On Mon, 2012-04-16 at 14:16 +0200, Jiri Olsa wrote:
>> group:        group_spec '{' events '}'
>> group_spec:   name  ':' mod | empty 
>
> How about something like:
>
> groups: groups ',' group | group
> group:	group_name '{' events '}' group_mod
> group_name: name ':' | empty
> group_mod: ':' group_mods | empty
> group_mods: [l]

Wouldn't it be better if group_mods accepts [ukhHGp] also and pass them
to all members in the group?

Thanks,
Namhyung

Re: [RFCv2 0/8] perf tool: Add new event group management

[Jiri Olsa]

On Tue, Apr 17, 2012 at 06:09:11PM +0900, Namhyung Kim wrote:
> Hi,
> 
> On Mon, 16 Apr 2012 17:26:02 +0200, Peter Zijlstra wrote:
> > On Mon, 2012-04-16 at 14:16 +0200, Jiri Olsa wrote:
> >> group:        group_spec '{' events '}'
> >> group_spec:   name  ':' mod | empty 
> >
> > How about something like:
> >
> > groups: groups ',' group | group
> > group:	group_name '{' events '}' group_mod
> > group_name: name ':' | empty
> > group_mod: ':' group_mods | empty
> > group_mods: [l]
> 
> Wouldn't it be better if group_mods accepts [ukhHGp] also and pass them
> to all members in the group?

nice idea, I think it's doable

jirka

Re: [RFCv2 0/8] perf tool: Add new event group management

[Andi Kleen]

On Wed, Apr 04, 2012 at 11:16:08PM +0200, Jiri Olsa wrote:
> hi,
> adding support for creating event groups based on the way they
> are specified on the command line. 

Any updates on this patchkit? We have a situation here where better 
group management is needed to avoid problems with inconsistent
counters that depend on each others.

I don't see it in tip.

-Andi

Re: [RFCv2 0/8] perf tool: Add new event group management

[Jiri Olsa]

On Sat, May 26, 2012 at 12:36:46AM +0200, Andi Kleen wrote:
> On Wed, Apr 04, 2012 at 11:16:08PM +0200, Jiri Olsa wrote:
> > hi,
> > adding support for creating event groups based on the way they
> > are specified on the command line. 
> 
> Any updates on this patchkit? We have a situation here where better 
> group management is needed to avoid problems with inconsistent
> counters that depend on each others.

The startup patches just got in recently
http://marc.info/?l=linux-kernel&m=133758460912306&w=2

so I'll continue on this shortly.. 

If you have some ideas on this or real world examples,
that would really help.. so far, here's the latest discussion:
http://marc.info/?t=133357436900005&r=1&w=2

and here's some thought on hows the display's going to look like:
http://marc.info/?l=linux-kernel&m=133458636513108&w=2


jirka

Re: [RFCv2 0/8] perf tool: Add new event group management

[Andi Kleen]

On Sat, May 26, 2012 at 02:38:58PM +0200, Jiri Olsa wrote:
> The startup patches just got in recently
> http://marc.info/?l=linux-kernel&m=133758460912306&w=2
> 
> so I'll continue on this shortly.. 

Great.

> If you have some ideas on this or real world examples,

Any of the proposed syntaxes looked fine for me. The important
part is that it works in some form.

> that would really help.. so far, here's the latest discussion:
> http://marc.info/?t=133357436900005&r=1&w=2

For example you want to measure sandy bridge frontend contention in a 
more useful way than the dubious event in standard perf.

The formula for this is 

N = 4*CPU_CLK_UNHALTED.THREAD           (4 execution slots) 
Percent_FE_bound = 100*(IDQ_UOPS_NOT_DELIVERED.CORE / N)

Translated into perf this is 

-e r53003c -e r53019c

and some glue to compute the formula:

#!/usr/bin/python
import sys

cyc, e1 = sys.stdin.readline().split(",")
uops, e2 = sys.stdin.readline().split(",")

N = 4 * float(cyc) 
P_FE = 100.0 * (float(uops) / N)
print "percent frontend bound: %.2f" % (P_FE)


perf stat -x, -e r53003c -e r53019c /bin/ls 2>log
./frontend.py < log
percent frontend bound: 41.53

My /bin/ls is 42% frontend bound.

Now you see we always have to measure the CPU_CLK_UNHALTED and 
IDQ_UOPS_NOT_DELIVERED.CORE together. Otherwise there is no useful output
from the formula.

The problem happens when we want to measure other things too. You tend
to quickly run out of 4 counters per CPU thread, so have to multiplex.
And that is where the groups are needed. Without the groups we have
to do multiple runs, instead of one that measures this all time sliced.

This is pretty common with all kinds of measurements.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFCv2 0/8] perf tool: Add new event group management

[Ulrich Drepper]

On Sat, May 26, 2012 at 8:38 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> If you have some ideas on this or real world examples,
> that would really help.. so far, here's the latest discussion:
> http://marc.info/?t=133357436900005&r=1&w=2

If you're looking for a definitive source, just point to the Intel
optimization manual.  Absolute values of counters are not really
useful and so they are defining many (50+) ratios which people should
investigate.  These ratios are only really accurate if the counters
are swapped in and out at the same time.

The reminds me of a detail I looked at when starting an an
implementation for this (glad you got more time to devote to it).  The
problem with ratios are that there are so many.  So efficient
scheduling is going to be important.  Many ratios use as a base the
same counters over and over again (e.g., cycle count, instruction
count, etc).  Therefore it is important to recognize when two groups
can be scheduled concurrently even if the total number of counters
needed would be high but due to intersections it is possible.

One last comment, not critical.  From a parsing point of view the
colon in the proposed syntax

    name : { counter1, counter2 }

is unnecessary.  Just one more thing people can get wrong.  How about
leaving it out?  An open curly brace to indicate a group should be
sufficient.

Re: [RFCv2 0/8] perf tool: Add new event group management

[Andi Kleen]

> same counters over and over again (e.g., cycle count, instruction
> count, etc).  Therefore it is important to recognize when two groups

These two are luckily fixed counters, so always available
(at least as long as you disable the nmi watchdog)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFCv2 0/8] perf tool: Add new event group management

[Jiri Olsa]

On Sun, May 27, 2012 at 03:56:22AM -0400, Ulrich Drepper wrote:
> On Sat, May 26, 2012 at 8:38 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > If you have some ideas on this or real world examples,
> > that would really help.. so far, here's the latest discussion:
> > http://marc.info/?t=133357436900005&r=1&w=2
> 
> If you're looking for a definitive source, just point to the Intel
> optimization manual.  Absolute values of counters are not really
> useful and so they are defining many (50+) ratios which people should
> investigate.  These ratios are only really accurate if the counters
> are swapped in and out at the same time.

thanks a lot for the pointer, very useful

> 
> The reminds me of a detail I looked at when starting an an
> implementation for this (glad you got more time to devote to it).  The
> problem with ratios are that there are so many.  So efficient
> scheduling is going to be important.  Many ratios use as a base the
> same counters over and over again (e.g., cycle count, instruction
> count, etc).  Therefore it is important to recognize when two groups
> can be scheduled concurrently even if the total number of counters
> needed would be high but due to intersections it is possible.
> 
> One last comment, not critical.  From a parsing point of view the
> colon in the proposed syntax
> 
>     name : { counter1, counter2 }
> 
> is unnecessary.  Just one more thing people can get wrong.  How about
> leaving it out?  An open curly brace to indicate a group should be
> sufficient.

yep, we'll omit the first colon

I'll CC you guys on next patchset

thanks,
jirka

Re: [RFCv2 0/8] perf tool: Add new event group management

[Peter Zijlstra]

On Sun, 2012-05-27 at 03:56 -0400, Ulrich Drepper wrote:
> Therefore it is important to recognize when two groups
> can be scheduled concurrently even if the total number of counters
> needed would be high but due to intersections it is possible. 

We currently don't do counter multiplexing like that. I'd love to see a
readable and efficient implementation of that though, preferably in the
core and not the arch code.