[PATCH V2 0/6] perf: Driver specific configuration for PMU

[PATCH V2 0/6] perf: Driver specific configuration for PMU

[Mathieu Poirier]

This patchset adds the possiblity of specifying PMU driver configuration
directly from the perf command line.  Anything that falls within the
event specifiers '/.../' and that is preceeded by the '@' symbol is
treated as a configurable.  Two formats are supported, @cfg and
@cfg=config.

For example:

perf record -e some_event/@cfg1/ ...

or

perf record -e some_event/@cfg2=config/ ...

or

perf record -e some_event/@cfg1, at cfg2=config/ ...

The above are all valid configuration and will see the strings 'cfg1'
and 'cfg2=config' sent to the PMU driver for parsing and interpretation
using the existing ioctl() mechanism.

The primary customers for this feature are the CoreSight drivers where
the selection of a sink (where trace data is accumulated) needs to be
done in a previous, and separated step, from the launching of the perf
command.

As such something that used to be a two-step process:

# echo 1 > /sys/bus/coresight/devices/20070000.etr/enable_sink
# perf record -e cs_etm//u --per-thread  uname

is integrated in a single command:

# perf record -e cs_etm/@sink=20070000.etr/u --per-thread  uname

The patches include both the kernel and user space part so that the
solution is complete and found in a single place.

It is based on [1] and assumes this set [2] has been applied.

Thanks,
Mathieu

Changes for V2:
- Rebased to [1] as per Jiri's request.

[1]. git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/core
[2]. https://lkml.org/lkml/2016/7/20/519

Mathieu Poirier (6):
  perf/core: Adding PMU driver specific configuration
  perf: Passing struct perf_event to function setup_aux()
  perf tools: add infrastructure for PMU specific configuration
  perf tools: pushing driver configuration down to the kernel
  coresight: adding sink parameter to function coresight_build_path()
  coresight: etm-perf: incorporating sink definition from cmd line

 arch/x86/events/intel/bts.c                      |   4 +-
 arch/x86/events/intel/pt.c                       |   5 +-
 drivers/hwtracing/coresight/coresight-etm-perf.c | 105 ++++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-priv.h     |   3 +-
 drivers/hwtracing/coresight/coresight.c          |  40 ++++++---
 include/linux/perf_event.h                       |  11 ++-
 include/uapi/linux/perf_event.h                  |   1 +
 kernel/events/core.c                             |  16 ++++
 kernel/events/ring_buffer.c                      |   2 +-
 tools/include/uapi/linux/perf_event.h            |   1 +
 tools/perf/builtin-record.c                      |   9 ++
 tools/perf/util/evlist.c                         |  24 ++++++
 tools/perf/util/evlist.h                         |   3 +
 tools/perf/util/evsel.c                          |  33 +++++++
 tools/perf/util/evsel.h                          |   7 ++
 tools/perf/util/parse-events.c                   |  76 +++++++++++-----
 tools/perf/util/parse-events.h                   |   1 +
 tools/perf/util/parse-events.l                   |  12 +++
 tools/perf/util/parse-events.y                   |  11 +++
 19 files changed, 321 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCH V2 1/6] perf/core: Adding PMU driver specific configuration

[Mathieu Poirier]

This patch somewhat mimics the work done on address filters to
add the infrastructure needed to pass PMU specific HW
configuration to the driver before a session starts.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/linux/perf_event.h            |  9 +++++++++
 include/uapi/linux/perf_event.h       |  1 +
 kernel/events/core.c                  | 16 ++++++++++++++++
 tools/include/uapi/linux/perf_event.h |  1 +
 4 files changed, 27 insertions(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 7921f4f20a58..59d61a12cf9d 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -168,6 +168,9 @@ struct hw_perf_event {
 	/* Last sync'ed generation of filters */
 	unsigned long			addr_filters_gen;
 
+	/* HW specific configuration */
+	void				*drv_configs;
+
 /*
  * hw_perf_event::state flags; used to track the PERF_EF_* state.
  */
@@ -442,6 +445,12 @@ struct pmu {
 	 * Filter events for PMU-specific reasons.
 	 */
 	int (*filter_match)		(struct perf_event *event); /* optional */
+
+	/*
+	 * PMU driver specific configuration.
+	 */
+	int (*set_drv_configs)		(struct perf_event *event,
+					 void __user *arg); /* optional */
 };
 
 /**
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index c66a485a24ac..90fbc5fd3925 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -407,6 +407,7 @@ struct perf_event_attr {
 #define PERF_EVENT_IOC_ID		_IOR('$', 7, __u64 *)
 #define PERF_EVENT_IOC_SET_BPF		_IOW('$', 8, __u32)
 #define PERF_EVENT_IOC_PAUSE_OUTPUT	_IOW('$', 9, __u32)
+#define PERF_EVENT_IOC_SET_DRV_CONFIGS	_IOW('$', 10, char *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 79dae188a987..9208e6ec036f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4457,6 +4457,8 @@ static int perf_event_set_output(struct perf_event *event,
 				 struct perf_event *output_event);
 static int perf_event_set_filter(struct perf_event *event, void __user *arg);
 static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
+static int perf_event_set_drv_configs(struct perf_event *event,
+				      void __user *arg);
 
 static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned long arg)
 {
@@ -4526,6 +4528,10 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
 		rcu_read_unlock();
 		return 0;
 	}
+
+	case PERF_EVENT_IOC_SET_DRV_CONFIGS:
+		return perf_event_set_drv_configs(event, (void __user *)arg);
+
 	default:
 		return -ENOTTY;
 	}
@@ -4558,6 +4564,7 @@ static long perf_compat_ioctl(struct file *file, unsigned int cmd,
 	switch (_IOC_NR(cmd)) {
 	case _IOC_NR(PERF_EVENT_IOC_SET_FILTER):
 	case _IOC_NR(PERF_EVENT_IOC_ID):
+	case _IOC_NR(PERF_EVENT_IOC_SET_DRV_CONFIGS):
 		/* Fix up pointer size (usually 4 -> 8 in 32-on-64-bit case */
 		if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
 			cmd &= ~IOCSIZE_MASK;
@@ -7633,6 +7640,15 @@ void perf_bp_event(struct perf_event *bp, void *data)
 }
 #endif
 
+static int perf_event_set_drv_configs(struct perf_event *event,
+				  void __user *arg)
+{
+	if (!event->pmu->set_drv_configs)
+		return -EINVAL;
+
+	return event->pmu->set_drv_configs(event, arg);
+}
+
 /*
  * Allocate a new address filter
  */
diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index c66a485a24ac..90fbc5fd3925 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -407,6 +407,7 @@ struct perf_event_attr {
 #define PERF_EVENT_IOC_ID		_IOR('$', 7, __u64 *)
 #define PERF_EVENT_IOC_SET_BPF		_IOW('$', 8, __u32)
 #define PERF_EVENT_IOC_PAUSE_OUTPUT	_IOW('$', 9, __u32)
+#define PERF_EVENT_IOC_SET_DRV_CONFIGS	_IOW('$', 10, char *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
-- 
2.7.4

[PATCH V2 2/6] perf: Passing struct perf_event to function setup_aux()

[Mathieu Poirier]

Some information, like driver specific configuration, is found
in the hw_perf_event  structure.  As such pass a 'struct perf_event'
to function setup_aux() rather than just the CPU number so that
individual drivers can make the right configuration when setting
up a session.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/events/intel/bts.c                      | 4 +++-
 arch/x86/events/intel/pt.c                       | 5 +++--
 drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++--
 include/linux/perf_event.h                       | 2 +-
 kernel/events/ring_buffer.c                      | 2 +-
 5 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 0a6e393a2e62..98155c2dfcce 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -68,8 +68,10 @@ static size_t buf_size(struct page *page)
 }
 
 static void *
-bts_buffer_setup_aux(int cpu, void **pages, int nr_pages, bool overwrite)
+bts_buffer_setup_aux(struct perf_event *event, void **pages,
+		     int nr_pages, bool overwrite)
 {
+	int cpu = event->cpu;
 	struct bts_buffer *buf;
 	struct page *page;
 	int node = (cpu == -1) ? cpu : cpu_to_node(cpu);
diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 04bb5fb5a8d7..5178c5de0b19 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1003,10 +1003,11 @@ static int pt_buffer_init_topa(struct pt_buffer *buf, unsigned long nr_pages,
  * Return:	Our private PT buffer structure.
  */
 static void *
-pt_buffer_setup_aux(int cpu, void **pages, int nr_pages, bool snapshot)
+pt_buffer_setup_aux(struct perf_event *event, void **pages,
+		    int nr_pages, bool snapshot)
 {
 	struct pt_buffer *buf;
-	int node, ret;
+	int node, ret, cpu = event->cpu;
 
 	if (!nr_pages)
 		return NULL;
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 755125f7917f..f4174f36c5a0 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -155,7 +155,7 @@ static void etm_free_aux(void *data)
 	schedule_work(&event_data->work);
 }
 
-static void *etm_setup_aux(int event_cpu, void **pages,
+static void *etm_setup_aux(struct perf_event *event, void **pages,
 			   int nr_pages, bool overwrite)
 {
 	int cpu;
@@ -163,7 +163,7 @@ static void *etm_setup_aux(int event_cpu, void **pages,
 	struct coresight_device *sink;
 	struct etm_event_data *event_data = NULL;
 
-	event_data = alloc_event_data(event_cpu);
+	event_data = alloc_event_data(event->cpu);
 	if (!event_data)
 		return NULL;
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 59d61a12cf9d..5275b6594989 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -407,7 +407,7 @@ struct pmu {
 	/*
 	 * Set up pmu-private data structures for an AUX area
 	 */
-	void *(*setup_aux)		(int cpu, void **pages,
+	void *(*setup_aux)		(struct perf_event *event, void **pages,
 					 int nr_pages, bool overwrite);
 					/* optional */
 
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index ae9b90dc9a5a..56aba90af437 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -616,7 +616,7 @@ int rb_alloc_aux(struct ring_buffer *rb, struct perf_event *event,
 			goto out;
 	}
 
-	rb->aux_priv = event->pmu->setup_aux(event->cpu, rb->aux_pages, nr_pages,
+	rb->aux_priv = event->pmu->setup_aux(event, rb->aux_pages, nr_pages,
 					     overwrite);
 	if (!rb->aux_priv)
 		goto out;
-- 
2.7.4

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

This patchset adds PMU driver specific configuration to the parser
infrastructure by preceding any term with the '@' letter.  As such
doing something like:

perf record -e some_event/@cfg1, at cfg2=config/ ...

will see 'cfg1' and 'cfg2=config' being added to the list of evsel config
terms.  Token 'cfg1' and 'cfg2=config' are not processed in user space
and are meant to be interpreted by the PMU driver.

First the lexer/parser are supplemented with the required definitions to
recognise the driver specific configuration.  From there they are simply
added to the list of event terms.  The bulk of the work is done in
function "parse_events_add_pmu()" where driver config event terms are
added to a new list of driver config terms, which in turn spliced with
the event's new driver configuration list.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/util/evsel.c        |  1 +
 tools/perf/util/evsel.h        |  4 +++
 tools/perf/util/parse-events.c | 76 +++++++++++++++++++++++++++++++-----------
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/parse-events.l | 12 +++++++
 tools/perf/util/parse-events.y | 11 ++++++
 6 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 8c54df61fe64..b16f1621ce3e 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -231,6 +231,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
 	evsel->bpf_fd	   = -1;
 	INIT_LIST_HEAD(&evsel->node);
 	INIT_LIST_HEAD(&evsel->config_terms);
+	INIT_LIST_HEAD(&evsel->drv_config_terms);
 	perf_evsel__object.init(evsel);
 	evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
 	perf_evsel__calc_id_pos(evsel);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 8a4a6c9f1480..e25fd5e4c740 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -46,6 +46,7 @@ enum {
 	PERF_EVSEL__CONFIG_TERM_INHERIT,
 	PERF_EVSEL__CONFIG_TERM_MAX_STACK,
 	PERF_EVSEL__CONFIG_TERM_OVERWRITE,
+	PERF_EVSEL__CONFIG_TERM_DRV_CFG,
 	PERF_EVSEL__CONFIG_TERM_MAX,
 };
 
@@ -57,6 +58,7 @@ struct perf_evsel_config_term {
 		u64	freq;
 		bool	time;
 		char	*callgraph;
+		char	*drv_cfg;
 		u64	stack_user;
 		int	max_stack;
 		bool	inherit;
@@ -79,6 +81,7 @@ struct perf_evsel_config_term {
  *          PERF_SAMPLE_IDENTIFIER) in a non-sample event i.e. if sample_id_all
  *          is used there is an id sample appended to non-sample events
  * @priv:   And what is in its containing unnamed union are tool specific
+ * @drv_config_terms: List of configurables sent directly to the PMU driver
  */
 struct perf_evsel {
 	struct list_head	node;
@@ -125,6 +128,7 @@ struct perf_evsel {
 	char			*group_name;
 	bool			cmdline_group_boundary;
 	struct list_head	config_terms;
+	struct list_head	drv_config_terms;
 	int			bpf_fd;
 };
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 6c913c3914fb..697a21c11fc5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -303,7 +303,8 @@ static struct perf_evsel *
 __add_event(struct list_head *list, int *idx,
 	    struct perf_event_attr *attr,
 	    char *name, struct cpu_map *cpus,
-	    struct list_head *config_terms)
+	    struct list_head *config_terms,
+	    struct list_head *drv_config_terms)
 {
 	struct perf_evsel *evsel;
 
@@ -322,6 +323,10 @@ __add_event(struct list_head *list, int *idx,
 	if (config_terms)
 		list_splice(config_terms, &evsel->config_terms);
 
+	if (drv_config_terms)
+		list_splice(drv_config_terms, &evsel->drv_config_terms);
+
+
 	list_add_tail(&evsel->node, list);
 	return evsel;
 }
@@ -330,7 +335,8 @@ static int add_event(struct list_head *list, int *idx,
 		     struct perf_event_attr *attr, char *name,
 		     struct list_head *config_terms)
 {
-	return __add_event(list, idx, attr, name, NULL, config_terms) ? 0 : -ENOMEM;
+	return __add_event(list, idx, attr, name,
+			   NULL, config_terms, NULL) ? 0 : -ENOMEM;
 }
 
 static int parse_aliases(char *str, const char *names[][PERF_EVSEL__MAX_ALIASES], int size)
@@ -904,6 +910,7 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
 	[PARSE_EVENTS__TERM_TYPE_MAX_STACK]		= "max-stack",
 	[PARSE_EVENTS__TERM_TYPE_OVERWRITE]		= "overwrite",
 	[PARSE_EVENTS__TERM_TYPE_NOOVERWRITE]		= "no-overwrite",
+	[PARSE_EVENTS__TERM_TYPE_DRV_CFG]		= "driver-config",
 };
 
 static bool config_term_shrinked;
@@ -1034,7 +1041,8 @@ static int config_term_pmu(struct perf_event_attr *attr,
 			   struct parse_events_term *term,
 			   struct parse_events_error *err)
 {
-	if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER)
+	if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER ||
+	    term->type_term == PARSE_EVENTS__TERM_TYPE_DRV_CFG)
 		/*
 		 * Always succeed for sysfs terms, as we dont know
 		 * at this point what type they need to have.
@@ -1083,10 +1091,7 @@ static int config_attr(struct perf_event_attr *attr,
 	return 0;
 }
 
-static int get_config_terms(struct list_head *head_config,
-			    struct list_head *head_terms __maybe_unused)
-{
-#define ADD_CONFIG_TERM(__type, __name, __val)			\
+#define ADD_CONFIG_TERM(__type, __name, __val, __head_terms)	\
 do {								\
 	struct perf_evsel_config_term *__t;			\
 								\
@@ -1097,42 +1102,53 @@ do {								\
 	INIT_LIST_HEAD(&__t->list);				\
 	__t->type       = PERF_EVSEL__CONFIG_TERM_ ## __type;	\
 	__t->val.__name = __val;				\
-	list_add_tail(&__t->list, head_terms);			\
+	list_add_tail(&__t->list, __head_terms);		\
 } while (0)
 
+static int get_config_terms(struct list_head *head_config,
+			    struct list_head *head_terms)
+{
 	struct parse_events_term *term;
 
 	list_for_each_entry(term, head_config, list) {
 		switch (term->type_term) {
 		case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
-			ADD_CONFIG_TERM(PERIOD, period, term->val.num);
+			ADD_CONFIG_TERM(PERIOD, period,
+					term->val.num, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
-			ADD_CONFIG_TERM(FREQ, freq, term->val.num);
+			ADD_CONFIG_TERM(FREQ, freq, term->val.num, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_TIME:
-			ADD_CONFIG_TERM(TIME, time, term->val.num);
+			ADD_CONFIG_TERM(TIME, time, term->val.num, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
-			ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
+			ADD_CONFIG_TERM(CALLGRAPH, callgraph,
+					term->val.str, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
-			ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
+			ADD_CONFIG_TERM(STACK_USER, stack_user,
+					term->val.num, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_INHERIT:
-			ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
+			ADD_CONFIG_TERM(INHERIT, inherit,
+					term->val.num ? 1 : 0, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
-			ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
+			ADD_CONFIG_TERM(INHERIT, inherit,
+					term->val.num ? 0 : 1, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
-			ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
+			ADD_CONFIG_TERM(MAX_STACK, max_stack,
+					term->val.num, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
-			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
+			ADD_CONFIG_TERM(OVERWRITE, overwrite,
+					term->val.num ? 1 : 0, head_terms);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
-			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
+			ADD_CONFIG_TERM(OVERWRITE, overwrite,
+					term->val.num ? 0 : 1, head_terms);
 			break;
 		default:
 			break;
@@ -1142,6 +1158,21 @@ do {								\
 	return 0;
 }
 
+static int get_drv_config_terms(struct list_head *head_config,
+				struct list_head *head_terms)
+{
+	struct parse_events_term *term;
+
+	list_for_each_entry(term, head_config, list) {
+		if (term->type_term != PARSE_EVENTS__TERM_TYPE_DRV_CFG)
+			continue;
+
+		ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str, head_terms);
+	}
+
+	return 0;
+}
+
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				const char *sys, const char *event,
 				struct parse_events_error *err,
@@ -1197,6 +1228,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 	struct perf_pmu *pmu;
 	struct perf_evsel *evsel;
 	LIST_HEAD(config_terms);
+	LIST_HEAD(drv_config_terms);
 
 	pmu = perf_pmu__find(name);
 	if (!pmu)
@@ -1211,7 +1243,8 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 
 	if (!head_config) {
 		attr.type = pmu->type;
-		evsel = __add_event(list, &data->idx, &attr, NULL, pmu->cpus, NULL);
+		evsel = __add_event(list, &data->idx, &attr,
+				    NULL, pmu->cpus, NULL, NULL);
 		return evsel ? 0 : -ENOMEM;
 	}
 
@@ -1228,12 +1261,15 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 	if (get_config_terms(head_config, &config_terms))
 		return -ENOMEM;
 
+	if (get_drv_config_terms(head_config, &drv_config_terms))
+		return -ENOMEM;
+
 	if (perf_pmu__config(pmu, &attr, head_config, data->error))
 		return -EINVAL;
 
 	evsel = __add_event(list, &data->idx, &attr,
 			    get_config_name(head_config), pmu->cpus,
-			    &config_terms);
+			    &config_terms, &drv_config_terms);
 	if (evsel) {
 		evsel->unit = info.unit;
 		evsel->scale = info.scale;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index d1edbf8cc66a..8d09a976fca8 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -71,6 +71,7 @@ enum {
 	PARSE_EVENTS__TERM_TYPE_MAX_STACK,
 	PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
 	PARSE_EVENTS__TERM_TYPE_OVERWRITE,
+	PARSE_EVENTS__TERM_TYPE_DRV_CFG,
 	__PARSE_EVENTS__TERM_TYPE_NR,
 };
 
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 7a2519435da0..1f7e11a6c5b3 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
 	return token;
 }
 
+static int drv_str(yyscan_t scanner, int token)
+{
+	YYSTYPE *yylval = parse_events_get_lval(scanner);
+	char *text = parse_events_get_text(scanner);
+
+	/* Strip off the '@' */
+	yylval->str = strdup(text + 1);
+	return token;
+}
+
 #define REWIND(__alloc)				\
 do {								\
 	YYSTYPE *__yylval = parse_events_get_lval(yyscanner);	\
@@ -124,6 +134,7 @@ num_hex		0x[a-fA-F0-9]+
 num_raw_hex	[a-fA-F0-9]+
 name		[a-zA-Z_*?][a-zA-Z0-9_*?.]*
 name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
+drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
 /* If you add a modifier you need to update check_modifier() */
 modifier_event	[ukhpPGHSDI]+
 modifier_bp	[rwx]{1,3}
@@ -209,6 +220,7 @@ no-overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 {name_minus}		{ return str(yyscanner, PE_NAME); }
 \[all\]			{ return PE_ARRAY_ALL; }
 "["			{ BEGIN(array); return '['; }
+@{drv_cfg_term}		{ return drv_str(yyscanner, PE_DRV_CFG_TERM); }
 }
 
 <mem>{
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 5be4a5f216d6..879115f93edc 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -49,6 +49,7 @@ static void inc_group_count(struct list_head *list,
 %token PE_ERROR
 %token PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
 %token PE_ARRAY_ALL PE_ARRAY_RANGE
+%token PE_DRV_CFG_TERM
 %type <num> PE_VALUE
 %type <num> PE_VALUE_SYM_HW
 %type <num> PE_VALUE_SYM_SW
@@ -63,6 +64,7 @@ static void inc_group_count(struct list_head *list,
 %type <str> PE_MODIFIER_BP
 %type <str> PE_EVENT_NAME
 %type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
+%type <str> PE_DRV_CFG_TERM
 %type <num> value_sym
 %type <head> event_config
 %type <head> opt_event_config
@@ -599,6 +601,15 @@ PE_NAME array '=' PE_VALUE
 	term->array = $2;
 	$$ = term;
 }
+|
+PE_DRV_CFG_TERM
+{
+	struct parse_events_term *term;
+
+	ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
+					$1, $1, &@1, NULL));
+	$$ = term;
+}
 
 array:
 '[' array_terms ']'
-- 
2.7.4

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:
> This patchset adds PMU driver specific configuration to the parser
> infrastructure by preceding any term with the '@' letter.  As such
> doing something like:
> 
> perf record -e some_event/@cfg1, at cfg2=config/ ...
> 
> will see 'cfg1' and 'cfg2=config' being added to the list of evsel config
> terms.  Token 'cfg1' and 'cfg2=config' are not processed in user space
> and are meant to be interpreted by the PMU driver.

please also update the docs, like 'Documentation/perf-record.txt --event option'

thanks,
jirka

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:

SNIP

> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index d1edbf8cc66a..8d09a976fca8 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -71,6 +71,7 @@ enum {
>  	PARSE_EVENTS__TERM_TYPE_MAX_STACK,
>  	PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
>  	PARSE_EVENTS__TERM_TYPE_OVERWRITE,
> +	PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>  	__PARSE_EVENTS__TERM_TYPE_NR,
>  };
>  
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 7a2519435da0..1f7e11a6c5b3 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>  	return token;
>  }
>  
> +static int drv_str(yyscan_t scanner, int token)
> +{
> +	YYSTYPE *yylval = parse_events_get_lval(scanner);
> +	char *text = parse_events_get_text(scanner);
> +
> +	/* Strip off the '@' */
> +	yylval->str = strdup(text + 1);
> +	return token;
> +}

why don't let bison parse this with rule like:

| '@' PE_DRV_CFG_TERM
{
...
}


you could omit the drv_str function then

thanks,
jirka

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 01:47, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>> index d1edbf8cc66a..8d09a976fca8 100644
>> --- a/tools/perf/util/parse-events.h
>> +++ b/tools/perf/util/parse-events.h
>> @@ -71,6 +71,7 @@ enum {
>>       PARSE_EVENTS__TERM_TYPE_MAX_STACK,
>>       PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
>>       PARSE_EVENTS__TERM_TYPE_OVERWRITE,
>> +     PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>>       __PARSE_EVENTS__TERM_TYPE_NR,
>>  };
>>
>> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>> index 7a2519435da0..1f7e11a6c5b3 100644
>> --- a/tools/perf/util/parse-events.l
>> +++ b/tools/perf/util/parse-events.l
>> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>>       return token;
>>  }
>>
>> +static int drv_str(yyscan_t scanner, int token)
>> +{
>> +     YYSTYPE *yylval = parse_events_get_lval(scanner);
>> +     char *text = parse_events_get_text(scanner);
>> +
>> +     /* Strip off the '@' */
>> +     yylval->str = strdup(text + 1);
>> +     return token;
>> +}
>
> why don't let bison parse this with rule like:
>
> | '@' PE_DRV_CFG_TERM
> {
> ...
> }
>
>
> you could omit the drv_str function then

I simply thought it was simple and clean to do it that way - and it
follows the trend already in place.  Are you sure you want me to move
this to bison?  Either way we have to add code...

Many thanks for the review,
Mathieu

>
> thanks,
> jirka

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:

SNIP

> >> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> >> index 7a2519435da0..1f7e11a6c5b3 100644
> >> --- a/tools/perf/util/parse-events.l
> >> +++ b/tools/perf/util/parse-events.l
> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
> >>       return token;
> >>  }
> >>
> >> +static int drv_str(yyscan_t scanner, int token)
> >> +{
> >> +     YYSTYPE *yylval = parse_events_get_lval(scanner);
> >> +     char *text = parse_events_get_text(scanner);
> >> +
> >> +     /* Strip off the '@' */
> >> +     yylval->str = strdup(text + 1);
> >> +     return token;
> >> +}
> >
> > why don't let bison parse this with rule like:
> >
> > | '@' PE_DRV_CFG_TERM
> > {
> > ...
> > }
> >
> >
> > you could omit the drv_str function then
> 
> I simply thought it was simple and clean to do it that way - and it
> follows the trend already in place.  Are you sure you want me to move
> this to bison?  Either way we have to add code...
> 
> Many thanks for the review,
> Mathieu

hum, i might be missing something, but what I meant
was something like below

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 1f7e11a6c5b3..9b00f9b9caa2 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
 	return token;
 }
 
-static int drv_str(yyscan_t scanner, int token)
-{
-	YYSTYPE *yylval = parse_events_get_lval(scanner);
-	char *text = parse_events_get_text(scanner);
-
-	/* Strip off the '@' */
-	yylval->str = strdup(text + 1);
-	return token;
-}
-
 #define REWIND(__alloc)				\
 do {								\
 	YYSTYPE *__yylval = parse_events_get_lval(yyscanner);	\
@@ -220,7 +210,7 @@ no-overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 {name_minus}		{ return str(yyscanner, PE_NAME); }
 \[all\]			{ return PE_ARRAY_ALL; }
 "["			{ BEGIN(array); return '['; }
-@{drv_cfg_term}		{ return drv_str(yyscanner, PE_DRV_CFG_TERM); }
+{drv_cfg_term}		{ return PE_DRV_CFG_TERM; }
 }
 
 <mem>{
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 879115f93edc..b7af1b834fda 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
 	$$ = term;
 }
 |
-PE_DRV_CFG_TERM
+'@' PE_DRV_CFG_TERM
 {
 	struct parse_events_term *term;
 
 	ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
-					$1, $1, &@1, NULL));
+					$2, $2, &@2, NULL));
 	$$ = term;
 }
 

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 08:54, Jiri Olsa <jolsa@redhat.com> wrote:
> On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> >> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>> >> index 7a2519435da0..1f7e11a6c5b3 100644
>> >> --- a/tools/perf/util/parse-events.l
>> >> +++ b/tools/perf/util/parse-events.l
>> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>> >>       return token;
>> >>  }
>> >>
>> >> +static int drv_str(yyscan_t scanner, int token)
>> >> +{
>> >> +     YYSTYPE *yylval = parse_events_get_lval(scanner);
>> >> +     char *text = parse_events_get_text(scanner);
>> >> +
>> >> +     /* Strip off the '@' */
>> >> +     yylval->str = strdup(text + 1);
>> >> +     return token;
>> >> +}
>> >
>> > why don't let bison parse this with rule like:
>> >
>> > | '@' PE_DRV_CFG_TERM
>> > {
>> > ...
>> > }
>> >
>> >
>> > you could omit the drv_str function then
>>
>> I simply thought it was simple and clean to do it that way - and it
>> follows the trend already in place.  Are you sure you want me to move
>> this to bison?  Either way we have to add code...
>>
>> Many thanks for the review,
>> Mathieu
>
> hum, i might be missing something, but what I meant
> was something like below
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..9b00f9b9caa2 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
>         return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -       YYSTYPE *yylval = parse_events_get_lval(scanner);
> -       char *text = parse_events_get_text(scanner);
> -
> -       /* Strip off the '@' */
> -       yylval->str = strdup(text + 1);
> -       return token;
> -}
> -
>  #define REWIND(__alloc)                                \
>  do {                                                           \
>         YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -220,7 +210,7 @@ no-overwrite                { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  {name_minus}           { return str(yyscanner, PE_NAME); }
>  \[all\]                        { return PE_ARRAY_ALL; }
>  "["                    { BEGIN(array); return '['; }
> -@{drv_cfg_term}                { return drv_str(yyscanner, PE_DRV_CFG_TERM); }
> +{drv_cfg_term}         { return PE_DRV_CFG_TERM; }
>  }
>
>  <mem>{
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 879115f93edc..b7af1b834fda 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
>         $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_DRV_CFG_TERM
>  {
>         struct parse_events_term *term;
>
>         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -                                       $1, $1, &@1, NULL));
> +                                       $2, $2, &@2, NULL));
>         $$ = term;
>  }
>

Yes, this might just work too.  Let me play around a little to make
sure all the cases are covered.

Thanks,
Mathieu

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 08:54, Jiri Olsa <jolsa@redhat.com> wrote:
> On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> >> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>> >> index 7a2519435da0..1f7e11a6c5b3 100644
>> >> --- a/tools/perf/util/parse-events.l
>> >> +++ b/tools/perf/util/parse-events.l
>> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>> >>       return token;
>> >>  }
>> >>
>> >> +static int drv_str(yyscan_t scanner, int token)
>> >> +{
>> >> +     YYSTYPE *yylval = parse_events_get_lval(scanner);
>> >> +     char *text = parse_events_get_text(scanner);
>> >> +
>> >> +     /* Strip off the '@' */
>> >> +     yylval->str = strdup(text + 1);
>> >> +     return token;
>> >> +}
>> >
>> > why don't let bison parse this with rule like:
>> >
>> > | '@' PE_DRV_CFG_TERM
>> > {
>> > ...
>> > }
>> >
>> >
>> > you could omit the drv_str function then
>>
>> I simply thought it was simple and clean to do it that way - and it
>> follows the trend already in place.  Are you sure you want me to move
>> this to bison?  Either way we have to add code...
>>
>> Many thanks for the review,
>> Mathieu
>
> hum, i might be missing something, but what I meant
> was something like below
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..9b00f9b9caa2 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
>         return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -       YYSTYPE *yylval = parse_events_get_lval(scanner);
> -       char *text = parse_events_get_text(scanner);
> -
> -       /* Strip off the '@' */
> -       yylval->str = strdup(text + 1);
> -       return token;
> -}
> -
>  #define REWIND(__alloc)                                \
>  do {                                                           \
>         YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -220,7 +210,7 @@ no-overwrite                { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  {name_minus}           { return str(yyscanner, PE_NAME); }
>  \[all\]                        { return PE_ARRAY_ALL; }
>  "["                    { BEGIN(array); return '['; }
> -@{drv_cfg_term}                { return drv_str(yyscanner, PE_DRV_CFG_TERM); }
> +{drv_cfg_term}         { return PE_DRV_CFG_TERM; }
>  }
>
>  <mem>{
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
>         $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_DRV_CFG_TERM
>  {
>         struct parse_events_term *term;
>
>         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -                                       $1, $1, &@1, NULL));
> +                                       $2, $2, &@2, NULL));
>         $$ = term;
>  }
>

I've been experimenting with the above solution and it is not yielding
the results one might think at first glance.

If we use the example: -e event/@cfg1/ ...

First if we leave things exactly the way they are suggested in the
code snippet flex doesn't know what do to with the '@' character and
returns an error.  To deal with that a new clause

"@"        { return '@'; }

can be inserted in the config state.  But that doesn't link '@' with
'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
state upon hitting '@' would get us around that but we are moving away
from our initial goal of keeping things simple.

Another solution is to have something like this:

drv_cfg_term    @[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?

But then we need to strip off the '@' character in the parse-events.y
file, which isn't pretty given that it can be done so easily with the
solution I had before.

So I'm a little puzzled on how to proceed here, but I'm well aware
that my flex/bison knowledge might also be too shallow...

Thanks,
Mathieu

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Fri, Jul 22, 2016 at 12:24:48PM -0600, Mathieu Poirier wrote:

SNIP

> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> >         $$ = term;
> >  }
> >  |
> > -PE_DRV_CFG_TERM
> > +'@' PE_DRV_CFG_TERM
> >  {
> >         struct parse_events_term *term;
> >
> >         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> > -                                       $1, $1, &@1, NULL));
> > +                                       $2, $2, &@2, NULL));
> >         $$ = term;
> >  }
> >
> 
> I've been experimenting with the above solution and it is not yielding
> the results one might think at first glance.
> 
> If we use the example: -e event/@cfg1/ ...
> 
> First if we leave things exactly the way they are suggested in the
> code snippet flex doesn't know what do to with the '@' character and
> returns an error.  To deal with that a new clause
> 
> "@"        { return '@'; }
> 
> can be inserted in the config state.  But that doesn't link '@' with
> 'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
> state upon hitting '@' would get us around that but we are moving away
> from our initial goal of keeping things simple.

hum, then how about keeping the flex atoms simple like for the
other terms and do something like below.. untested ;-)

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 1f7e11a6c5b3..8ba228e1c150 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
 	return token;
 }
 
-static int drv_str(yyscan_t scanner, int token)
-{
-	YYSTYPE *yylval = parse_events_get_lval(scanner);
-	char *text = parse_events_get_text(scanner);
-
-	/* Strip off the '@' */
-	yylval->str = strdup(text + 1);
-	return token;
-}
-
 #define REWIND(__alloc)				\
 do {								\
 	YYSTYPE *__yylval = parse_events_get_lval(yyscanner);	\
@@ -134,7 +124,6 @@ num_hex		0x[a-fA-F0-9]+
 num_raw_hex	[a-fA-F0-9]+
 name		[a-zA-Z_*?][a-zA-Z0-9_*?.]*
 name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
-drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
 /* If you add a modifier you need to update check_modifier() */
 modifier_event	[ukhpPGHSDI]+
 modifier_bp	[rwx]{1,3}
@@ -216,11 +205,11 @@ no-inherit		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
 overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
 no-overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 ,			{ return ','; }
+"@"			{ return '@'; }
 "/"			{ BEGIN(INITIAL); return '/'; }
 {name_minus}		{ return str(yyscanner, PE_NAME); }
 \[all\]			{ return PE_ARRAY_ALL; }
 "["			{ BEGIN(array); return '['; }
-@{drv_cfg_term}		{ return drv_str(yyscanner, PE_DRV_CFG_TERM); }
 }
 
 <mem>{
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 879115f93edc..7e03e93dabca 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
 	$$ = term;
 }
 |
-PE_DRV_CFG_TERM
+'@' PE_NAME '=' PE_NAME
 {
 	struct parse_events_term *term;
 
 	ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
-					$1, $1, &@1, NULL));
+					$2, $4, &@2, &@4));
 	$$ = term;
 }
 

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 26 July 2016 at 14:41, Jiri Olsa <jolsa@redhat.com> wrote:
> On Fri, Jul 22, 2016 at 12:24:48PM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>> > --- a/tools/perf/util/parse-events.y
>> > +++ b/tools/perf/util/parse-events.y
>> > @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
>> >         $$ = term;
>> >  }
>> >  |
>> > -PE_DRV_CFG_TERM
>> > +'@' PE_DRV_CFG_TERM
>> >  {
>> >         struct parse_events_term *term;
>> >
>> >         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>> > -                                       $1, $1, &@1, NULL));
>> > +                                       $2, $2, &@2, NULL));
>> >         $$ = term;
>> >  }
>> >
>>
>> I've been experimenting with the above solution and it is not yielding
>> the results one might think at first glance.
>>
>> If we use the example: -e event/@cfg1/ ...
>>
>> First if we leave things exactly the way they are suggested in the
>> code snippet flex doesn't know what do to with the '@' character and
>> returns an error.  To deal with that a new clause
>>
>> "@"        { return '@'; }
>>
>> can be inserted in the config state.  But that doesn't link '@' with
>> 'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
>> state upon hitting '@' would get us around that but we are moving away
>> from our initial goal of keeping things simple.
>
> hum, then how about keeping the flex atoms simple like for the
> other terms and do something like below.. untested ;-)
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..8ba228e1c150 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
>         return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -       YYSTYPE *yylval = parse_events_get_lval(scanner);
> -       char *text = parse_events_get_text(scanner);
> -
> -       /* Strip off the '@' */
> -       yylval->str = strdup(text + 1);
> -       return token;
> -}
> -
>  #define REWIND(__alloc)                                \
>  do {                                                           \
>         YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -134,7 +124,6 @@ num_hex             0x[a-fA-F0-9]+
>  num_raw_hex    [a-fA-F0-9]+
>  name           [a-zA-Z_*?][a-zA-Z0-9_*?.]*
>  name_minus     [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
> -drv_cfg_term   [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
>  /* If you add a modifier you need to update check_modifier() */
>  modifier_event [ukhpPGHSDI]+
>  modifier_bp    [rwx]{1,3}
> @@ -216,11 +205,11 @@ no-inherit                { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
>  overwrite              { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
>  no-overwrite           { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  ,                      { return ','; }
> +"@"                    { return '@'; }
>  "/"                    { BEGIN(INITIAL); return '/'; }
>  {name_minus}           { return str(yyscanner, PE_NAME); }
>  \[all\]                        { return PE_ARRAY_ALL; }
>  "["                    { BEGIN(array); return '['; }
> -@{drv_cfg_term}                { return drv_str(yyscanner, PE_DRV_CFG_TERM); }
>  }
>
>  <mem>{
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 879115f93edc..7e03e93dabca 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
>         $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_NAME '=' PE_NAME
>  {
>         struct parse_events_term *term;
>
>         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -                                       $1, $1, &@1, NULL));
> +                                       $2, $4, &@2, &@4));
>         $$ = term;
>  }
>

The problem here is that the correlation between the first and the
second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
kernel only gets the value associated with the second PE_NAME.

For example,

-e event/@cfg1=value1, at cfg2=value2/ ...

The above code will send "value1" and "value2" to the kernel driver
where there is no way to know what configurable the values correspond
to.  To go around that we'd have to concatenate $2 and $4 in function
parse_events_term__str() (or new_term()) when @type_term ==
PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
hackish to me.

Thanks,
Mathieu

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:

SNIP

> > -PE_DRV_CFG_TERM
> > +'@' PE_NAME '=' PE_NAME
> >  {
> >         struct parse_events_term *term;
> >
> >         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> > -                                       $1, $1, &@1, NULL));
> > +                                       $2, $4, &@2, &@4));
> >         $$ = term;
> >  }
> >
> 
> The problem here is that the correlation between the first and the
> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
> kernel only gets the value associated with the second PE_NAME.
> 
> For example,
> 
> -e event/@cfg1=value1, at cfg2=value2/ ...
> 
> The above code will send "value1" and "value2" to the kernel driver
> where there is no way to know what configurable the values correspond

hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?

jirka

> to.  To go around that we'd have to concatenate $2 and $4 in function
> parse_events_term__str() (or new_term()) when @type_term ==
> PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
> hackish to me.
> 
> Thanks,
> Mathieu

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 27 July 2016 at 13:26, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> > -PE_DRV_CFG_TERM
>> > +'@' PE_NAME '=' PE_NAME
>> >  {
>> >         struct parse_events_term *term;
>> >
>> >         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>> > -                                       $1, $1, &@1, NULL));
>> > +                                       $2, $4, &@2, &@4));
>> >         $$ = term;
>> >  }
>> >
>>
>> The problem here is that the correlation between the first and the
>> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
>> kernel only gets the value associated with the second PE_NAME.
>>
>> For example,
>>
>> -e event/@cfg1=value1, at cfg2=value2/ ...
>>
>> The above code will send "value1" and "value2" to the kernel driver
>> where there is no way to know what configurable the values correspond
>
> hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?

Indeed you do.

Macro ADD_CONFIG_TERM in function get_config_terms() only account for
the __val parameter and struct parse_events_term::config is completely
ignored.  We could concatenate the fields before calling
ADD_CONFIG_TERM() but between that and freeing the reserved memory, I
think it is cleaner to let flex do the work.

Mathieu

>
> jirka
>
>> to.  To go around that we'd have to concatenate $2 and $4 in function
>> parse_events_term__str() (or new_term()) when @type_term ==
>> PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
>> hackish to me.
>>
>> Thanks,
>> Mathieu

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Thu, Jul 28, 2016 at 10:15:52AM -0600, Mathieu Poirier wrote:
> On 27 July 2016 at 13:26, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:
> >
> > SNIP
> >
> >> > -PE_DRV_CFG_TERM
> >> > +'@' PE_NAME '=' PE_NAME
> >> >  {
> >> >         struct parse_events_term *term;
> >> >
> >> >         ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> >> > -                                       $1, $1, &@1, NULL));
> >> > +                                       $2, $4, &@2, &@4));
> >> >         $$ = term;
> >> >  }
> >> >
> >>
> >> The problem here is that the correlation between the first and the
> >> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
> >> kernel only gets the value associated with the second PE_NAME.
> >>
> >> For example,
> >>
> >> -e event/@cfg1=value1, at cfg2=value2/ ...
> >>
> >> The above code will send "value1" and "value2" to the kernel driver
> >> where there is no way to know what configurable the values correspond
> >
> > hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?
> 
> Indeed you do.
> 
> Macro ADD_CONFIG_TERM in function get_config_terms() only account for
> the __val parameter and struct parse_events_term::config is completely
> ignored.  We could concatenate the fields before calling
> ADD_CONFIG_TERM() but between that and freeing the reserved memory, I
> think it is cleaner to let flex do the work.

ah you need that whole string in one piece.. ok
let's do it by your original way then

please add some comments for in drv_str function describing
the usage of @ and the fact we're doing this because we need
whole assignment as single string

thanks,
jirka

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:

SNIP

> +static int get_config_terms(struct list_head *head_config,
> +			    struct list_head *head_terms)
> +{
>  	struct parse_events_term *term;
>  
>  	list_for_each_entry(term, head_config, list) {
>  		switch (term->type_term) {
>  		case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> -			ADD_CONFIG_TERM(PERIOD, period, term->val.num);
> +			ADD_CONFIG_TERM(PERIOD, period,
> +					term->val.num, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> -			ADD_CONFIG_TERM(FREQ, freq, term->val.num);
> +			ADD_CONFIG_TERM(FREQ, freq, term->val.num, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_TIME:
> -			ADD_CONFIG_TERM(TIME, time, term->val.num);
> +			ADD_CONFIG_TERM(TIME, time, term->val.num, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> -			ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
> +			ADD_CONFIG_TERM(CALLGRAPH, callgraph,
> +					term->val.str, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> -			ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
> +			ADD_CONFIG_TERM(STACK_USER, stack_user,
> +					term->val.num, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_INHERIT:
> -			ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
> +			ADD_CONFIG_TERM(INHERIT, inherit,
> +					term->val.num ? 1 : 0, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> -			ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
> +			ADD_CONFIG_TERM(INHERIT, inherit,
> +					term->val.num ? 0 : 1, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> -			ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
> +			ADD_CONFIG_TERM(MAX_STACK, max_stack,
> +					term->val.num, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> -			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> +			ADD_CONFIG_TERM(OVERWRITE, overwrite,
> +					term->val.num ? 1 : 0, head_terms);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> -			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> +			ADD_CONFIG_TERM(OVERWRITE, overwrite,
> +					term->val.num ? 0 : 1, head_terms);
>  			break;

I think it'd be better to have all terms altogether,
and handle the 'case PARSE_EVENTS__TERM_TYPE_DRV_CFG:'
in here, rather than in separate function

also ADD_CONFIG_TERM macro could stay local to the
function with no need of adding new head arguments

thanks,
jirka

>  		default:
>  			break;
> @@ -1142,6 +1158,21 @@ do {								\
>  	return 0;
>  }
>  
> +static int get_drv_config_terms(struct list_head *head_config,
> +				struct list_head *head_terms)
> +{
> +	struct parse_events_term *term;
> +
> +	list_for_each_entry(term, head_config, list) {
> +		if (term->type_term != PARSE_EVENTS__TERM_TYPE_DRV_CFG)
> +			continue;
> +
> +		ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str, head_terms);
> +	}
> +
> +	return 0;
> +}

SNIP

[PATCH V2 4/6] perf tools: pushing driver configuration down to the kernel

[Mathieu Poirier]

Now that PMU specific driver configuration are queued in
evsel::drv_config_terms, all we need to do is re-use the current
ioctl() mechanism to push down the information to the kernel
driver.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/builtin-record.c |  9 +++++++++
 tools/perf/util/evlist.c    | 24 ++++++++++++++++++++++++
 tools/perf/util/evlist.h    |  3 +++
 tools/perf/util/evsel.c     | 32 ++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.h     |  3 +++
 5 files changed, 71 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 8f2c16d9275f..dffea1033b8e 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -383,6 +383,7 @@ static int record__open(struct record *rec)
 	struct perf_evlist *evlist = rec->evlist;
 	struct perf_session *session = rec->session;
 	struct record_opts *opts = &rec->opts;
+	struct perf_evsel_config_term *err_term;
 	int rc = 0;
 
 	perf_evlist__config(evlist, opts, &callchain_param);
@@ -412,6 +413,14 @@ try_again:
 		goto out;
 	}
 
+	if (perf_evlist__apply_drv_configs(evlist, &pos, &err_term)) {
+		error("failed to set config \"%s\" on event %s with %d (%s)\n",
+			err_term->val.drv_cfg, perf_evsel__name(pos), errno,
+			strerror_r(errno, msg, sizeof(msg)));
+		rc = -1;
+		goto out;
+	}
+
 	rc = record__mmap(rec);
 	if (rc)
 		goto out;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 2a40b8e1def7..0f485cebae18 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1433,6 +1433,30 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e
 	return err;
 }
 
+int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
+				   struct perf_evsel **err_evsel,
+				   struct perf_evsel_config_term **err_term)
+{
+	struct perf_evsel *evsel;
+	int err = 0;
+	const int ncpus = cpu_map__nr(evlist->cpus),
+		  nthreads = thread_map__nr(evlist->threads);
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (list_empty(&evsel->drv_config_terms))
+			continue;
+
+		err = perf_evsel__apply_drv_configs(evsel, ncpus,
+						    nthreads, err_term);
+		if (err) {
+			*err_evsel = evsel;
+			break;
+		}
+	}
+
+	return err;
+}
+
 int perf_evlist__set_filter(struct perf_evlist *evlist, const char *filter)
 {
 	struct perf_evsel *evsel;
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 4fd034f22d2f..bf7ed0be3f33 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -232,6 +232,9 @@ void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,
 			   struct thread_map *threads);
 int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target);
 int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel);
+int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
+				   struct perf_evsel **err_evsel,
+				   struct perf_evsel_config_term **term);
 
 void __perf_evlist__set_leader(struct list_head *list);
 void perf_evlist__set_leader(struct perf_evlist *evlist);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index b16f1621ce3e..e5b30d02a5dd 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1035,6 +1035,27 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 	return -1;
 }
 
+int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
+				  int ncpus, int nthreads,
+				  struct perf_evsel_config_term **err_term)
+{
+	int err = 0;
+	struct perf_evsel_config_term *term;
+
+	list_for_each_entry(term, &evsel->drv_config_terms, list) {
+		err = perf_evsel__run_ioctl(evsel, ncpus, nthreads,
+					    PERF_EVENT_IOC_SET_DRV_CONFIGS,
+					    (void *)term->val.drv_cfg);
+
+		if (err) {
+			*err_term = term;
+			break;
+		}
+	}
+
+	return err;
+}
+
 int perf_evsel__enable(struct perf_evsel *evsel)
 {
 	int nthreads = thread_map__nr(evsel->threads);
@@ -1100,6 +1121,16 @@ static void perf_evsel__free_config_terms(struct perf_evsel *evsel)
 	}
 }
 
+static void perf_evsel__free_drv_config_terms(struct perf_evsel *evsel)
+{
+	struct perf_evsel_config_term *term, *h;
+
+	list_for_each_entry_safe(term, h, &evsel->drv_config_terms, list) {
+		list_del(&term->list);
+		free(term);
+	}
+}
+
 void perf_evsel__close_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
 {
 	int cpu, thread;
@@ -1121,6 +1152,7 @@ void perf_evsel__exit(struct perf_evsel *evsel)
 	perf_evsel__free_fd(evsel);
 	perf_evsel__free_id(evsel);
 	perf_evsel__free_config_terms(evsel);
+	perf_evsel__free_drv_config_terms(evsel);
 	close_cgroup(evsel->cgrp);
 	cpu_map__put(evsel->cpus);
 	cpu_map__put(evsel->own_cpus);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index e25fd5e4c740..305b2e11992a 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -239,6 +239,9 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 			      const char *op, const char *filter);
 int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
 			     const char *filter);
+int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
+				  int ncpus, int nthreads,
+				  struct perf_evsel_config_term **err_term);
 int perf_evsel__enable(struct perf_evsel *evsel);
 int perf_evsel__disable(struct perf_evsel *evsel);
 
-- 
2.7.4

[PATCH V2 4/6] perf tools: pushing driver configuration down to the kernel

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:18PM -0600, Mathieu Poirier wrote:
> Now that PMU specific driver configuration are queued in
> evsel::drv_config_terms, all we need to do is re-use the current
> ioctl() mechanism to push down the information to the kernel
> driver.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  tools/perf/builtin-record.c |  9 +++++++++
>  tools/perf/util/evlist.c    | 24 ++++++++++++++++++++++++
>  tools/perf/util/evlist.h    |  3 +++
>  tools/perf/util/evsel.c     | 32 ++++++++++++++++++++++++++++++++
>  tools/perf/util/evsel.h     |  3 +++
>  5 files changed, 71 insertions(+)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 8f2c16d9275f..dffea1033b8e 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -383,6 +383,7 @@ static int record__open(struct record *rec)
>  	struct perf_evlist *evlist = rec->evlist;
>  	struct perf_session *session = rec->session;
>  	struct record_opts *opts = &rec->opts;
> +	struct perf_evsel_config_term *err_term;
>  	int rc = 0;
>  
>  	perf_evlist__config(evlist, opts, &callchain_param);
> @@ -412,6 +413,14 @@ try_again:
>  		goto out;
>  	}
>  
> +	if (perf_evlist__apply_drv_configs(evlist, &pos, &err_term)) {
> +		error("failed to set config \"%s\" on event %s with %d (%s)\n",
> +			err_term->val.drv_cfg, perf_evsel__name(pos), errno,
> +			strerror_r(errno, msg, sizeof(msg)));
> +		rc = -1;
> +		goto out;
> +	}
> +

how about 'perf top' and 'perf stat', should they support this too?

thanks,
jirka

[PATCH V2 4/6] perf tools: pushing driver configuration down to the kernel

[Mathieu Poirier]

On 21 July 2016 at 01:47, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Jul 20, 2016 at 02:38:18PM -0600, Mathieu Poirier wrote:
>> Now that PMU specific driver configuration are queued in
>> evsel::drv_config_terms, all we need to do is re-use the current
>> ioctl() mechanism to push down the information to the kernel
>> driver.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  tools/perf/builtin-record.c |  9 +++++++++
>>  tools/perf/util/evlist.c    | 24 ++++++++++++++++++++++++
>>  tools/perf/util/evlist.h    |  3 +++
>>  tools/perf/util/evsel.c     | 32 ++++++++++++++++++++++++++++++++
>>  tools/perf/util/evsel.h     |  3 +++
>>  5 files changed, 71 insertions(+)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index 8f2c16d9275f..dffea1033b8e 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -383,6 +383,7 @@ static int record__open(struct record *rec)
>>       struct perf_evlist *evlist = rec->evlist;
>>       struct perf_session *session = rec->session;
>>       struct record_opts *opts = &rec->opts;
>> +     struct perf_evsel_config_term *err_term;
>>       int rc = 0;
>>
>>       perf_evlist__config(evlist, opts, &callchain_param);
>> @@ -412,6 +413,14 @@ try_again:
>>               goto out;
>>       }
>>
>> +     if (perf_evlist__apply_drv_configs(evlist, &pos, &err_term)) {
>> +             error("failed to set config \"%s\" on event %s with %d (%s)\n",
>> +                     err_term->val.drv_cfg, perf_evsel__name(pos), errno,
>> +                     strerror_r(errno, msg, sizeof(msg)));
>> +             rc = -1;
>> +             goto out;
>> +     }
>> +
>
> how about 'perf top' and 'perf stat', should they support this too?

After looking into this (hence the delayed reply) I'm not completely sure.

'perf stat' calls perf_evlist__apply_filters() and therefore the
semantic is likely to be close enough for PMU driver specific
configuration to make sense, should a particular use case lends itself
to it.  But that is certainly not the case for CoreSight PMUs.  Since
there is no current client for it, do you wish to see 'perf stat'
support this feature?

>From a quick glance at the code, filters in the context of 'perf top'
seem to be related to symbols rather than the tuning of events.  As
such I just can't see how driver specific configuration would fit in
that scheme.

Regards,
Mathieu


>
> thanks,
> jirka

[PATCH V2 5/6] coresight: adding sink parameter to function coresight_build_path()

[Mathieu Poirier]

Up to now function coresight_build_path() was counting on a sink to
have been selected (from sysFS) prior to being called.  This patch
adds a string argument so that a sink matching the argument can be
selected.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c |  2 +-
 drivers/hwtracing/coresight/coresight-priv.h     |  3 +-
 drivers/hwtracing/coresight/coresight.c          | 40 +++++++++++++++---------
 3 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index f4174f36c5a0..f8c7a8733b23 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -184,7 +184,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		 * list of devices from source to sink that can be
 		 * referenced later when the path is actually needed.
 		 */
-		event_data->path[cpu] = coresight_build_path(csdev);
+		event_data->path[cpu] = coresight_build_path(csdev, NULL);
 		if (!event_data->path[cpu])
 			goto err;
 	}
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index ad975c58080d..3cb574b3cdd9 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -94,7 +94,8 @@ static inline void CS_UNLOCK(void __iomem *addr)
 void coresight_disable_path(struct list_head *path);
 int coresight_enable_path(struct list_head *path, u32 mode);
 struct coresight_device *coresight_get_sink(struct list_head *path);
-struct list_head *coresight_build_path(struct coresight_device *csdev);
+struct list_head *coresight_build_path(struct coresight_device *csdev,
+				       const char *sink);
 void coresight_release_path(struct list_head *path);
 
 #ifdef CONFIG_CORESIGHT_SOURCE_ETM3X
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index d08d1ab9bba5..cbbb51a16dff 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -372,30 +372,41 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
  * _coresight_build_path - recursively build a path from a @csdev to a sink.
  * @csdev:	The device to start from.
  * @path:	The list to add devices to.
+ * @sink:	The name of the sink this path should connect with.
  *
- * The tree of Coresight device is traversed until an activated sink is
- * found.  From there the sink is added to the list along with all the
- * devices that led to that point - the end result is a list from source
- * to sink. In that list the source is the first device and the sink the
- * last one.
+ * The tree of Coresight device is traversed until an activated sink or
+ * the one specified by @sink is found.
+ * From there the sink is added to the list along with all the devices that
+ * led to that point - the end result is a list from source to sink. In that
+ * list the source is the first device and the sink the last one.
  */
 static int _coresight_build_path(struct coresight_device *csdev,
-				 struct list_head *path)
+				 struct list_head *path, const char *sink)
 {
 	int i;
 	bool found = false;
 	struct coresight_node *node;
 
-	/* An activated sink has been found.  Enqueue the element */
-	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
-	     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) && csdev->activated)
-		goto out;
+	/*
+	 * First see if we are dealing with a sink.  If we have one check if
+	 * it was selected via sysFS or the perf cmd line.
+	 */
+	if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+	    csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
+		/* Activated via perf cmd line */
+		if (sink && !strcmp(dev_name(&csdev->dev), sink))
+			goto out;
+		/* Activated via sysFS */
+		if (csdev->activated)
+			goto out;
+	}
 
 	/* Not a sink - recursively explore each port found on this element */
 	for (i = 0; i < csdev->nr_outport; i++) {
 		struct coresight_device *child_dev = csdev->conns[i].child_dev;
 
-		if (child_dev && _coresight_build_path(child_dev, path) == 0) {
+		if (child_dev &&
+		    _coresight_build_path(child_dev, path, sink) == 0) {
 			found = true;
 			break;
 		}
@@ -422,7 +433,8 @@ out:
 	return 0;
 }
 
-struct list_head *coresight_build_path(struct coresight_device *csdev)
+struct list_head *coresight_build_path(struct coresight_device *csdev,
+				       const char *sink)
 {
 	struct list_head *path;
 	int rc;
@@ -433,7 +445,7 @@ struct list_head *coresight_build_path(struct coresight_device *csdev)
 
 	INIT_LIST_HEAD(path);
 
-	rc = _coresight_build_path(csdev, path);
+	rc = _coresight_build_path(csdev, path, sink);
 	if (rc) {
 		kfree(path);
 		return ERR_PTR(rc);
@@ -508,7 +520,7 @@ int coresight_enable(struct coresight_device *csdev)
 	if (csdev->enable)
 		goto out;
 
-	path = coresight_build_path(csdev);
+	path = coresight_build_path(csdev, NULL);
 	if (IS_ERR(path)) {
 		pr_err("building path(s) failed\n");
 		ret = PTR_ERR(path);
-- 
2.7.4

[PATCH V2 5/6] coresight: adding sink parameter to function coresight_build_path()

[Suzuki K Poulose]

On 20/07/16 21:38, Mathieu Poirier wrote:
> Up to now function coresight_build_path() was counting on a sink to
> have been selected (from sysFS) prior to being called.  This patch
> adds a string argument so that a sink matching the argument can be
> selected.
>

>  static int _coresight_build_path(struct coresight_device *csdev,
> -				 struct list_head *path)
> +				 struct list_head *path, const char *sink)
>  {
>  	int i;
>  	bool found = false;
>  	struct coresight_node *node;
>
> -	/* An activated sink has been found.  Enqueue the element */
> -	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> -	     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) && csdev->activated)
> -		goto out;
> +	/*
> +	 * First see if we are dealing with a sink.  If we have one check if
> +	 * it was selected via sysFS or the perf cmd line.
> +	 */
> +	if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> +	    csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
> +		/* Activated via perf cmd line */
> +		if (sink && !strcmp(dev_name(&csdev->dev), sink))
> +			goto out;
> +		/* Activated via sysFS */
> +		if (csdev->activated)

When a sink is specified, should we skip an activated sink and continue to
find the specified one ? or at least fail with an error as we may not be using
the sink specified by the user ?
i.e may be :
		if (!sink && csdev->activated)
			goto out;

Suzuki

[PATCH V2 5/6] coresight: adding sink parameter to function coresight_build_path()

[Mathieu Poirier]

On 21 July 2016 at 04:49, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 20/07/16 21:38, Mathieu Poirier wrote:
>>
>> Up to now function coresight_build_path() was counting on a sink to
>> have been selected (from sysFS) prior to being called.  This patch
>> adds a string argument so that a sink matching the argument can be
>> selected.
>>
>
>>  static int _coresight_build_path(struct coresight_device *csdev,
>> -                                struct list_head *path)
>> +                                struct list_head *path, const char *sink)
>>  {
>>         int i;
>>         bool found = false;
>>         struct coresight_node *node;
>>
>> -       /* An activated sink has been found.  Enqueue the element */
>> -       if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
>> -            csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
>> csdev->activated)
>> -               goto out;
>> +       /*
>> +        * First see if we are dealing with a sink.  If we have one check
>> if
>> +        * it was selected via sysFS or the perf cmd line.
>> +        */
>> +       if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
>> +           csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
>> +               /* Activated via perf cmd line */
>> +               if (sink && !strcmp(dev_name(&csdev->dev), sink))
>> +                       goto out;
>> +               /* Activated via sysFS */
>> +               if (csdev->activated)
>
>
> When a sink is specified, should we skip an activated sink and continue to
> find the specified one ? or at least fail with an error as we may not be
> using
> the sink specified by the user ?
> i.e may be :
>                 if (!sink && csdev->activated)
>                         goto out;


I understand your point.  My goal though is to discourage people from
meddling in sysFS when using CS from the perf interface.  As such if
any code is to be added here, it would be to report an error when both
a sink has been specified from perf and activated from sysFS.

>
> Suzuki

[PATCH V2 6/6] coresight: etm-perf: incorporating sink definition from cmd line

[Mathieu Poirier]

Now that PMU specific configuration is available as part of the event,
lookup the sink identified by users from the perf command line and build
a path from source to sink.

With this functionality it is no longer required to select a sink in a
separate step (from sysFS) before a perf trace session can be started.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 101 ++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index f8c7a8733b23..5658a7411a66 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -22,6 +22,7 @@
 #include <linux/list.h>
 #include <linux/mm.h>
 #include <linux/init.h>
+#include <linux/parser.h>
 #include <linux/perf_event.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -71,11 +72,20 @@ static const struct attribute_group *etm_pmu_attr_groups[] = {
 
 static void etm_event_read(struct perf_event *event) {}
 
+static void etm_event_destroy(struct perf_event *event)
+{
+	kfree(event->hw.drv_configs);
+	event->hw.drv_configs = NULL;
+}
+
 static int etm_event_init(struct perf_event *event)
 {
 	if (event->attr.type != etm_pmu.type)
 		return -ENOENT;
 
+	event->destroy = etm_event_destroy;
+	event->hw.drv_configs = NULL;
+
 	return 0;
 }
 
@@ -159,6 +169,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 			   int nr_pages, bool overwrite)
 {
 	int cpu;
+	char *cmdl_sink;
 	cpumask_t *mask;
 	struct coresight_device *sink;
 	struct etm_event_data *event_data = NULL;
@@ -171,6 +182,12 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 
 	mask = &event_data->mask;
 
+	/*
+	 * If a sink was specified from the perf cmdline it will be part of
+	 * the event's drv_configs.
+	 */
+	cmdl_sink = (char *)event->hw.drv_configs;
+
 	/* Setup the path for each CPU in a trace session */
 	for_each_cpu(cpu, mask) {
 		struct coresight_device *csdev;
@@ -184,7 +201,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		 * list of devices from source to sink that can be
 		 * referenced later when the path is actually needed.
 		 */
-		event_data->path[cpu] = coresight_build_path(csdev, NULL);
+		event_data->path[cpu] = coresight_build_path(csdev, cmdl_sink);
 		if (!event_data->path[cpu])
 			goto err;
 	}
@@ -342,6 +359,87 @@ static void etm_event_del(struct perf_event *event, int mode)
 	etm_event_stop(event, PERF_EF_UPDATE);
 }
 
+enum {
+	ETM_TOKEN_SINK_CPU,
+	ETM_TOKEN_SINK,
+	ETM_TOKEN_ERR,
+};
+
+static const match_table_t drv_cfg_tokens = {
+	{ETM_TOKEN_SINK_CPU, "sink=cpu%d:%s"},
+	{ETM_TOKEN_SINK, "sink=%s"},
+	{ETM_TOKEN_ERR, NULL},
+};
+
+static int etm_set_drv_configs(struct perf_event *event, void __user *arg)
+{
+	char *config, *sink = NULL;
+	int cpu = -1, token, ret = 0;
+	substring_t args[MAX_OPT_ARGS];
+
+	/* Only one sink per event */
+	if (event->hw.drv_configs != NULL) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* Make user supplied input usable */
+	config = strndup_user(arg, PAGE_SIZE);
+	if (IS_ERR(config)) {
+		ret = PTR_ERR(config);
+		goto err;
+	}
+
+	/* See above declared @drv_cfg_tokens for the usable formats */
+	token = match_token(config, drv_cfg_tokens, args);
+	switch (token) {
+	case ETM_TOKEN_SINK:
+		/* Just a sink has been specified */
+		sink = match_strdup(&args[0]);
+		if (IS_ERR(sink)) {
+			ret = PTR_ERR(sink);
+			goto err;
+		}
+		break;
+	case ETM_TOKEN_SINK_CPU:
+		/* We have a sink and a CPU */
+
+		/* First get the cpu */
+		if (match_int(&args[0], &cpu)) {
+			ret = -EINVAL;
+			goto err;
+		}
+
+		/* Then the sink */
+		sink = match_strdup(&args[1]);
+		if (IS_ERR(sink)) {
+			ret = PTR_ERR(sink);
+			goto err;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/*
+	 * If the CPUs don't match the sink is destined to another path.  This
+	 * isn't as an error hence not setting @ret.
+	 */
+	if (event->cpu != cpu)
+		goto err;
+
+	/* We have a valid configuration */
+	event->hw.drv_configs = sink;
+
+out:
+	return ret;
+
+err:
+	kfree(sink);
+	goto out;
+}
+
 int etm_perf_symlink(struct coresight_device *csdev, bool link)
 {
 	char entry[sizeof("cpu9999999")];
@@ -383,6 +481,7 @@ static int __init etm_perf_init(void)
 	etm_pmu.stop		= etm_event_stop;
 	etm_pmu.add		= etm_event_add;
 	etm_pmu.del		= etm_event_del;
+	etm_pmu.set_drv_configs	= etm_set_drv_configs;
 
 	ret = perf_pmu_register(&etm_pmu, CORESIGHT_ETM_PMU_NAME, -1);
 	if (ret == 0)
-- 
2.7.4