[PATCH 0/6] perf: Add ioctl for PMU driver configuration

[PATCH 0/6] perf: Add ioctl for PMU driver configuration

[Mathieu Poirier]

This set follows what has been done for filters by adding an ioctl
to communicate PMU specific driver configuration to the kernel.

Patch 01 and 02 provide the functionality in user space while patch 04
and 05 take care of the kernel part.  Patches 03 and 06 use the newly
added functionality to communicate the selection of CoreSight sink
for each event.

This functionality is needed to identify the CoreSight sink to use when
working with CPU-wide trace scenarios.  It also paves the way to support
complex tracer configurations where dozens of trace parameters need to
be communicated to the CoreSight PMU driver.

Applies cleanly on v4.18-rc3.

Best regards,
Mathieu

Mathieu Poirier (6):
  perf tools: Use ioctl to communicate driver configuration to kernel
  perf tools: Make perf_evsel accessible to PMU driver configuration
    code
  perf tools: Use ioctl function to send sink configuration to kernel
  perf/aux: Make perf_event accessible to setup_aux()
  perf/core: Use ioctl to communicate driver configuration to kernel
  coresight: Use PMU driver configuration for sink selection

 arch/s390/kernel/perf_cpum_sf.c                  |   4 +-
 arch/x86/events/intel/bts.c                      |   4 +-
 arch/x86/events/intel/pt.c                       |   5 +-
 drivers/hwtracing/coresight/coresight-etm-perf.c | 134 ++++++++++++++++++++---
 drivers/hwtracing/coresight/coresight-etm-perf.h |  10 ++
 drivers/perf/arm_spe_pmu.c                       |   6 +-
 include/linux/perf_event.h                       |  56 +++++++++-
 include/uapi/linux/perf_event.h                  |   1 +
 kernel/events/core.c                             | 110 +++++++++++++++++++
 kernel/events/ring_buffer.c                      |   2 +-
 tools/include/uapi/linux/perf_event.h            |   1 +
 tools/perf/arch/arm/util/cs-etm.c                |  60 +++-------
 tools/perf/arch/arm/util/cs-etm.h                |   3 +-
 tools/perf/util/drv_configs.c                    |  30 ++---
 tools/perf/util/evsel.c                          |   7 ++
 tools/perf/util/evsel.h                          |   1 +
 tools/perf/util/pmu.h                            |   3 +-
 17 files changed, 345 insertions(+), 92 deletions(-)

-- 
2.7.4

[PATCH 1/6] perf tools: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

Following in the footsteps of what was done for filters, adding the
necessary mechanic needed to push down driver specific configuration
to the kernel using an ioctl. By proceeding this way PMU specific
configuration can be communicated to each event.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/uapi/linux/perf_event.h       | 1 +
 tools/include/uapi/linux/perf_event.h | 1 +
 tools/perf/util/evsel.c               | 7 +++++++
 tools/perf/util/evsel.h               | 1 +
 4 files changed, 10 insertions(+)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index b8e288a1f740..b5b3241877df 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -460,6 +460,7 @@ struct perf_event_query_bpf {
 #define PERF_EVENT_IOC_PAUSE_OUTPUT		_IOW('$', 9, __u32)
 #define PERF_EVENT_IOC_QUERY_BPF		_IOWR('$', 10, struct perf_event_query_bpf *)
 #define PERF_EVENT_IOC_MODIFY_ATTRIBUTES	_IOW('$', 11, struct perf_event_attr *)
+#define PERF_EVENT_IOC_SET_DRV_CONFIG		_IOW('$', 12, char *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index b8e288a1f740..b5b3241877df 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -460,6 +460,7 @@ struct perf_event_query_bpf {
 #define PERF_EVENT_IOC_PAUSE_OUTPUT		_IOW('$', 9, __u32)
 #define PERF_EVENT_IOC_QUERY_BPF		_IOWR('$', 10, struct perf_event_query_bpf *)
 #define PERF_EVENT_IOC_MODIFY_ATTRIBUTES	_IOW('$', 11, struct perf_event_attr *)
+#define PERF_EVENT_IOC_SET_DRV_CONFIG		_IOW('$', 12, char *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 94fce4f537e9..534aca4c642c 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1162,6 +1162,13 @@ static int perf_evsel__append_filter(struct perf_evsel *evsel,
 	return -1;
 }
 
+int perf_evsel__apply_drv_config(struct perf_evsel *evsel, const char *config)
+{
+	return perf_evsel__run_ioctl(evsel,
+				     PERF_EVENT_IOC_SET_DRV_CONFIG,
+				     (void *)config);
+}
+
 int perf_evsel__append_tp_filter(struct perf_evsel *evsel, const char *filter)
 {
 	return perf_evsel__append_filter(evsel, "(%s) && (%s)", filter);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d277930b19a1..0f671bd2a988 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -271,6 +271,7 @@ int perf_evsel__append_tp_filter(struct perf_evsel *evsel, const char *filter);
 int perf_evsel__append_addr_filter(struct perf_evsel *evsel,
 				   const char *filter);
 int perf_evsel__apply_filter(struct perf_evsel *evsel, const char *filter);
+int perf_evsel__apply_drv_config(struct perf_evsel *evsel, const char *config);
 int perf_evsel__enable(struct perf_evsel *evsel);
 int perf_evsel__disable(struct perf_evsel *evsel);
 
-- 
2.7.4

[PATCH 2/6] perf tools: Make perf_evsel accessible to PMU driver configuration code

[Mathieu Poirier]

Make structure perf_evsel available to the PMU driver configuration code.
That way function perf_evsel__apply_drv_config() can be used from within
that code and information pertaining to the 'perf_evsel_config_term' is
still available.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 22 +++++++++++++++++++++-
 tools/perf/arch/arm/util/cs-etm.h |  3 ++-
 tools/perf/util/drv_configs.c     | 30 +++++++-----------------------
 tools/perf/util/pmu.h             |  3 ++-
 4 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 2f595cd73da6..d8081c2e6d44 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -635,7 +635,7 @@ static int __printf(2, 3) cs_device__print_file(const char *name, const char *fm
 	return ret;
 }
 
-int cs_etm_set_drv_config(struct perf_evsel_config_term *term)
+static int cs_etm_set_drv_config_term(struct perf_evsel_config_term *term)
 {
 	int ret;
 	char enable_sink[ENABLE_SINK_MAX];
@@ -649,3 +649,23 @@ int cs_etm_set_drv_config(struct perf_evsel_config_term *term)
 
 	return 0;
 }
+
+int cs_etm_set_drv_config(struct perf_evsel *evsel,
+			  struct perf_evsel_config_term **err_term)
+{
+	int err = 0;
+	struct perf_evsel_config_term *term;
+
+	list_for_each_entry(term, &evsel->config_terms, list) {
+		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
+			continue;
+
+		err = cs_etm_set_drv_config_term(term);
+		if (err) {
+			*err_term = term;
+			break;
+		}
+	}
+
+	return err;
+}
diff --git a/tools/perf/arch/arm/util/cs-etm.h b/tools/perf/arch/arm/util/cs-etm.h
index 1a12e64f5127..a3f8dde6ccef 100644
--- a/tools/perf/arch/arm/util/cs-etm.h
+++ b/tools/perf/arch/arm/util/cs-etm.h
@@ -10,6 +10,7 @@
 #include "../../util/evsel.h"
 
 struct auxtrace_record *cs_etm_record_init(int *err);
-int cs_etm_set_drv_config(struct perf_evsel_config_term *term);
+int cs_etm_set_drv_config(struct perf_evsel *evsel,
+			  struct perf_evsel_config_term **err_term);
 
 #endif
diff --git a/tools/perf/util/drv_configs.c b/tools/perf/util/drv_configs.c
index eec754243f4d..f7c1bcf08549 100644
--- a/tools/perf/util/drv_configs.c
+++ b/tools/perf/util/drv_configs.c
@@ -25,7 +25,6 @@ perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
 {
 	bool found = false;
 	int err = 0;
-	struct perf_evsel_config_term *term;
 	struct perf_pmu *pmu = NULL;
 
 	while ((pmu = perf_pmu__scan(pmu)) != NULL)
@@ -34,29 +33,14 @@ perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
 			break;
 		}
 
-	list_for_each_entry(term, &evsel->config_terms, list) {
-		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
-			continue;
+	/*
+	 * No need to continue if we didn't get a match or if there is no
+	 * driver configuration function for this PMU.
+	 */
+	if (!found || !pmu->set_drv_config)
+		return err;
 
-		/*
-		 * We have a configuration term, report an error if we
-		 * can't find the PMU or if the PMU driver doesn't support
-		 * cmd line driver configuration.
-		 */
-		if (!found || !pmu->set_drv_config) {
-			err = -EINVAL;
-			*err_term = term;
-			break;
-		}
-
-		err = pmu->set_drv_config(term);
-		if (err) {
-			*err_term = term;
-			break;
-		}
-	}
-
-	return err;
+	return pmu->set_drv_config(evsel, err_term);
 }
 
 int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 76fecec7b3f9..47f44394042b 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -29,7 +29,8 @@ struct perf_pmu {
 	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
 	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
 	struct list_head list;    /* ELEM */
-	int (*set_drv_config)	(struct perf_evsel_config_term *term);
+	int (*set_drv_config)	(struct perf_evsel *evsel,
+				 struct perf_evsel_config_term **err_term);
 };
 
 struct perf_pmu_info {
-- 
2.7.4

[PATCH 3/6] perf tools: Use ioctl function to send sink configuration to kernel

[Mathieu Poirier]

Using sysFS to communicate sink information for a trace session doesn't
work when more than one CPU is involved in the scenario.  As such
communicate the sink information to each event by using the SET_DRV_CONFIG
ioctl command.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 54 ++++-----------------------------------
 1 file changed, 5 insertions(+), 49 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index d8081c2e6d44..cb0978eb7181 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -599,55 +599,10 @@ struct auxtrace_record *cs_etm_record_init(int *err)
 	return NULL;
 }
 
-static FILE *cs_device__open_file(const char *name)
+static int cs_etm_set_drv_config_term(struct perf_evsel *evsel,
+				      const char *term)
 {
-	struct stat st;
-	char path[PATH_MAX];
-	const char *sysfs;
-
-	sysfs = sysfs__mountpoint();
-	if (!sysfs)
-		return NULL;
-
-	snprintf(path, PATH_MAX,
-		 "%s" CS_BUS_DEVICE_PATH "%s", sysfs, name);
-
-	if (stat(path, &st) < 0)
-		return NULL;
-
-	return fopen(path, "w");
-
-}
-
-static int __printf(2, 3) cs_device__print_file(const char *name, const char *fmt, ...)
-{
-	va_list args;
-	FILE *file;
-	int ret = -EINVAL;
-
-	va_start(args, fmt);
-	file = cs_device__open_file(name);
-	if (file) {
-		ret = vfprintf(file, fmt, args);
-		fclose(file);
-	}
-	va_end(args);
-	return ret;
-}
-
-static int cs_etm_set_drv_config_term(struct perf_evsel_config_term *term)
-{
-	int ret;
-	char enable_sink[ENABLE_SINK_MAX];
-
-	snprintf(enable_sink, ENABLE_SINK_MAX, "%s/%s",
-		 term->val.drv_cfg, "enable_sink");
-
-	ret = cs_device__print_file(enable_sink, "%d", 1);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	return perf_evsel__apply_drv_config(evsel, term);
 }
 
 int cs_etm_set_drv_config(struct perf_evsel *evsel,
@@ -660,7 +615,8 @@ int cs_etm_set_drv_config(struct perf_evsel *evsel,
 		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
 			continue;
 
-		err = cs_etm_set_drv_config_term(term);
+		err = cs_etm_set_drv_config_term(evsel,
+						 term->val.drv_cfg);
 		if (err) {
 			*err_term = term;
 			break;
-- 
2.7.4

[PATCH 4/6] perf/aux: Make perf_event accessible to setup_aux()

[Mathieu Poirier]

It can be advantagous to have access to all the information conveyed by
a perf_event when setting up the AUX buffer, as it is the case when
dealing with PMU specific driver configuration communicated to the kernel
using an ioctl() call.

As such simply replace the cpu information by the complete perf_event
structure and change all affected customers.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 arch/s390/kernel/perf_cpum_sf.c                  | 4 ++--
 arch/x86/events/intel/bts.c                      | 4 +++-
 arch/x86/events/intel/pt.c                       | 5 +++--
 drivers/hwtracing/coresight/coresight-etm-perf.c | 6 +++---
 drivers/perf/arm_spe_pmu.c                       | 6 +++---
 include/linux/perf_event.h                       | 2 +-
 kernel/events/ring_buffer.c                      | 2 +-
 7 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index 0292d68e7dde..e06daaa08894 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -1601,8 +1601,8 @@ static void aux_buffer_free(void *data)
  *
  * Return the private AUX buffer structure if success or NULL if fails.
  */
-static void *aux_buffer_setup(int cpu, void **pages, int nr_pages,
-			      bool snapshot)
+static void *aux_buffer_setup(struct perf_event *event, void **pages,
+			      int nr_pages, bool snapshot);
 {
 	struct sf_buffer *sfb;
 	struct aux_buffer *aux;
diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 24ffa1e88cf9..7139f6bf27ad 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -77,10 +77,12 @@ 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)
 {
 	struct bts_buffer *buf;
 	struct page *page;
+	int cpu = event->cpu;
 	int node = (cpu == -1) ? cpu : cpu_to_node(cpu);
 	unsigned long offset;
 	size_t size = nr_pages << PAGE_SHIFT;
diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 8d016ce5b80d..8f4c98fdd03c 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1104,10 +1104,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 677695635211..0f5e03e4df22 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -181,15 +181,15 @@ 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;
+	int cpu = event->cpu;
 	cpumask_t *mask;
 	struct coresight_device *sink;
 	struct etm_event_data *event_data = NULL;
 
-	event_data = alloc_event_data(event_cpu);
+	event_data = alloc_event_data(cpu);
 	if (!event_data)
 		return NULL;
 	INIT_WORK(&event_data->work, free_event_data);
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 54ec278d2fc4..4dcd7bf14dcc 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -824,10 +824,10 @@ static void arm_spe_pmu_read(struct perf_event *event)
 {
 }
 
-static void *arm_spe_pmu_setup_aux(int cpu, void **pages, int nr_pages,
-				   bool snapshot)
+static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
+				   int nr_pages, bool snapshot)
 {
-	int i;
+	int i, cpu = event->cpu;
 	struct page **pglist;
 	struct arm_spe_pmu_buf *buf;
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1fa12887ec02..4d9c8f30ca6c 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -409,7 +409,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 5d3cf407e374..4c96c7575224 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -648,7 +648,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 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

This patch follows what has been done for filters by adding an ioctl()
option to communicate to the kernel arbitrary PMU specific configuration
that don't fit in the conventional struct perf_event_attr to the kernel.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/linux/perf_event.h |  54 ++++++++++++++++++++++
 kernel/events/core.c       | 110 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 4d9c8f30ca6c..6e06b63c262f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -178,6 +178,12 @@ struct hw_perf_event {
 	/* Last sync'ed generation of filters */
 	unsigned long			addr_filters_gen;
 
+	/* PMU driver configuration */
+	void				*drv_config;
+
+	/* Last sync'ed generation of driver config */
+	unsigned long			drv_config_gen;
+
 /*
  * hw_perf_event::state flags; used to track the PERF_EF_* state.
  */
@@ -447,6 +453,26 @@ struct pmu {
 	 * Filter events for PMU-specific reasons.
 	 */
 	int (*filter_match)		(struct perf_event *event); /* optional */
+
+	/*
+	 * Valiate complex PMU configuration that don't fit in the
+	 * perf_event_attr struct.  Returns a PMU specific pointer or an error
+	 * value < 0.
+	 *
+	 * As with addr_filters_validate(), runs in the context of the ioctl()
+	 * process and is not serialized with the rest of the PMU callbacks.
+	 */
+	void *(*drv_config_validate)	(struct perf_event *event,
+					 char *config_str);
+
+	/* Synchronize PMU driver configuration */
+	void (*drv_config_sync)		(struct perf_event *event);
+
+	/*
+	 * Release PMU specific configuration acquired by
+	 * drv_config_validate()
+	 */
+	void (*drv_config_free)		(void *drv_data);
 };
 
 enum perf_addr_filter_action_t {
@@ -489,6 +515,11 @@ struct perf_addr_filters_head {
 	unsigned int		nr_file_filters;
 };
 
+struct perf_drv_config {
+	void		*drv_config;
+	raw_spinlock_t	lock;
+};
+
 /**
  * enum perf_event_state - the states of a event
  */
@@ -668,6 +699,10 @@ struct perf_event {
 	unsigned long			*addr_filters_offs;
 	unsigned long			addr_filters_gen;
 
+	/* PMU driver specific configuration */
+	struct perf_drv_config		drv_config;
+	unsigned long			drv_config_gen;
+
 	void (*destroy)(struct perf_event *);
 	struct rcu_head			rcu_head;
 
@@ -1234,6 +1269,13 @@ static inline bool has_addr_filter(struct perf_event *event)
 	return event->pmu->nr_addr_filters;
 }
 
+static inline bool has_drv_config(struct perf_event *event)
+{
+	return event->pmu->drv_config_validate &&
+	       event->pmu->drv_config_sync &&
+	       event->pmu->drv_config_free;
+}
+
 /*
  * An inherited event uses parent's filters
  */
@@ -1248,7 +1290,19 @@ perf_event_addr_filters(struct perf_event *event)
 	return ifh;
 }
 
+static inline struct perf_drv_config *
+perf_event_get_drv_config(struct perf_event *event)
+{
+	struct perf_drv_config *cfg = &event->drv_config;
+
+	if (event->parent)
+		cfg = &event->parent->drv_config;
+
+	return cfg;
+}
+
 extern void perf_event_addr_filters_sync(struct perf_event *event);
+extern void perf_event_drv_config_sync(struct perf_event *event);
 
 extern int perf_output_begin(struct perf_output_handle *handle,
 			     struct perf_event *event, unsigned int size);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8f0434a9951a..701839866789 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2829,6 +2829,29 @@ void perf_event_addr_filters_sync(struct perf_event *event)
 }
 EXPORT_SYMBOL_GPL(perf_event_addr_filters_sync);
 
+/*
+ * PMU driver configuration works the same way as filter management above,
+ * but without the need to deal with memory mapping.  Driver configuration
+ * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
+ * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
+ * bring the configuration information within reach of the PMU.
+ */
+void perf_event_drv_config_sync(struct perf_event *event)
+{
+	struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
+
+	if (!has_drv_config(event))
+		return;
+
+	raw_spin_lock(&drv_config->lock);
+	if (event->drv_config_gen != event->hw.drv_config_gen) {
+		event->pmu->drv_config_sync(event);
+		event->hw.drv_config_gen = event->drv_config_gen;
+	}
+	raw_spin_unlock(&drv_config->lock);
+}
+EXPORT_SYMBOL_GPL(perf_event_drv_config_sync);
+
 static int _perf_event_refresh(struct perf_event *event, int refresh)
 {
 	/*
@@ -4410,6 +4433,7 @@ static bool exclusive_event_installable(struct perf_event *event,
 
 static void perf_addr_filters_splice(struct perf_event *event,
 				       struct list_head *head);
+static void perf_drv_config_splice(struct perf_event *event, void *drv_data);
 
 static void _free_event(struct perf_event *event)
 {
@@ -4440,6 +4464,7 @@ static void _free_event(struct perf_event *event)
 	perf_event_free_bpf_prog(event);
 	perf_addr_filters_splice(event, NULL);
 	kfree(event->addr_filters_offs);
+	perf_drv_config_splice(event, NULL);
 
 	if (event->destroy)
 		event->destroy(event);
@@ -5002,6 +5027,8 @@ static inline int perf_fget_light(int fd, struct fd *p)
 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_drv_config(struct perf_event *event,
+				     void __user *arg);
 static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
 static int perf_copy_attr(struct perf_event_attr __user *uattr,
 			  struct perf_event_attr *attr);
@@ -5088,6 +5115,10 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
 
 		return perf_event_modify_attr(event,  &new_attr);
 	}
+
+	case PERF_EVENT_IOC_SET_DRV_CONFIG:
+		return perf_event_set_drv_config(event, (void __user *)arg);
+
 	default:
 		return -ENOTTY;
 	}
@@ -9086,6 +9117,85 @@ static int perf_event_set_filter(struct perf_event *event, void __user *arg)
 	return ret;
 }
 
+static void perf_drv_config_splice(struct perf_event *event, void *drv_data)
+{
+	unsigned long flags;
+	void *old_drv_data;
+
+	if (!has_drv_config(event))
+		return;
+
+	/* Children take their configuration from their parent */
+	if (event->parent)
+		return;
+
+	raw_spin_lock_irqsave(&event->drv_config.lock, flags);
+
+	old_drv_data = event->drv_config.drv_config;
+	event->drv_config.drv_config = drv_data;
+
+	raw_spin_unlock_irqrestore(&event->drv_config.lock, flags);
+
+	event->pmu->drv_config_free(old_drv_data);
+}
+
+static void perf_event_drv_config_apply(struct perf_event *event)
+{
+	unsigned long flags;
+	struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
+
+	/* Notify event that a new configuration is available */
+	raw_spin_lock_irqsave(&drv_config->lock, flags);
+	event->drv_config_gen++;
+	raw_spin_unlock_irqrestore(&drv_config->lock, flags);
+}
+
+static int
+perf_event_process_drv_config(struct perf_event *event, char *config_str)
+{
+	int ret = -EINVAL;
+	void *drv_data;
+
+	/* Make sure ctx.mutex it held */
+	lockdep_assert_held(&event->ctx->mutex);
+
+	/* Children take their configuration from their parent */
+	if (WARN_ON_ONCE(event->parent))
+		goto out;
+
+	drv_data = event->pmu->drv_config_validate(event, config_str);
+	if (!drv_data)
+		goto out;
+	if (IS_ERR(drv_data)) {
+		ret = PTR_ERR(drv_data);
+		goto out;
+	}
+
+	perf_drv_config_splice(event, drv_data);
+
+	perf_event_for_each_child(event, perf_event_drv_config_apply);
+
+	ret = 0;
+out:
+	return ret;
+}
+
+static int perf_event_set_drv_config(struct perf_event *event, void __user *arg)
+{
+	int ret = -EINVAL;
+	char *config_str;
+
+	config_str = strndup_user(arg, PAGE_SIZE);
+	if (IS_ERR(config_str))
+		return PTR_ERR(config_str);
+
+	if (has_drv_config(event))
+		ret = perf_event_process_drv_config(event, config_str);
+
+	kfree(config_str);
+	return ret;
+}
+
 /*
  * hrtimer based swevent callback
  */
-- 
2.7.4

[PATCH 6/6] coresight: Use PMU driver configuration for sink selection

[Mathieu Poirier]

This patch uses the PMU driver configuration API to select a sink
for each event that is created (the old sysFS way of working is kept
around for backward compatibility).

By proceeding in this way a sink can be used by multiple sessions
without having to play games with entries in sysFS.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 128 ++++++++++++++++++++---
 drivers/hwtracing/coresight/coresight-etm-perf.h |  10 ++
 2 files changed, 126 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 0f5e03e4df22..37c8b97d0449 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -11,6 +11,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>
@@ -22,6 +23,8 @@
 static struct pmu etm_pmu;
 static bool etm_perf_up;
 
+#define CORESIGHT_DEVICE_MAX_NAME_LEN	256
+
 /**
  * struct etm_event_data - Coresight specifics associated to an event
  * @work:		Handle to free allocated memory outside IRQ context.
@@ -81,10 +84,29 @@ static int etm_addr_filters_alloc(struct perf_event *event)
 	return 0;
 }
 
+static int etm_drv_config_alloc(struct perf_event *event)
+{
+	char *sink;
+	int node = event->cpu == -1 ? -1 : cpu_to_node(event->cpu);
+
+	sink = kzalloc_node(CORESIGHT_DEVICE_MAX_NAME_LEN, GFP_KERNEL, node);
+	if (!sink)
+		return -ENOMEM;
+
+	if (event->parent)
+		memcpy(sink, event->parent->hw.addr_filters,
+		       CORESIGHT_DEVICE_MAX_NAME_LEN);
+
+	event->hw.drv_config = sink;
+
+	return 0;
+}
+
 static void etm_event_destroy(struct perf_event *event)
 {
 	kfree(event->hw.addr_filters);
 	event->hw.addr_filters = NULL;
+	kfree(event->hw.drv_config);
 }
 
 static int etm_event_init(struct perf_event *event)
@@ -100,6 +122,10 @@ static int etm_event_init(struct perf_event *event)
 	if (ret)
 		goto out;
 
+	ret = etm_drv_config_alloc(event);
+	if (ret)
+		goto out;
+
 	event->destroy = etm_event_destroy;
 out:
 	return ret;
@@ -181,6 +207,38 @@ static void etm_free_aux(void *data)
 	schedule_work(&event_data->work);
 }
 
+static struct coresight_device *etm_event_get_sink(struct perf_event *event)
+{
+	/*
+	 * Try the preferred method first, i.e passing the sink information
+	 * using the ioctl() method.
+	 */
+	if (event->hw.drv_config) {
+		struct device *dev;
+		struct coresight_device *sink;
+
+		dev = bus_find_device_by_name(&coresight_bustype, NULL,
+					      (char *)event->hw.drv_config);
+		if (dev) {
+			sink = to_coresight_device(dev);
+			/* Put reference from 'bus_find_device()' */
+			put_device(dev);
+			return sink;
+		}
+	}
+
+	/*
+	 * No luck with the above method, so we are working with an older user
+	 * space.  See if a sink has been set using sysFS.  If this is the case
+	 * CPU-wide session will only be able to use a single sink.
+	 *
+	 * When operated from sysFS users are responsible to enable the sink
+	 * while from perf, the perf tools will do it based on the choice made
+	 * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
+	 */
+	return coresight_get_enabled_sink(true);
+}
+
 static void *etm_setup_aux(struct perf_event *event, void **pages,
 			   int nr_pages, bool overwrite)
 {
@@ -194,18 +252,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		return NULL;
 	INIT_WORK(&event_data->work, free_event_data);
 
-	/*
-	 * In theory nothing prevent tracers in a trace session from being
-	 * associated with different sinks, nor having a sink per tracer.  But
-	 * until we have HW with this kind of topology we need to assume tracers
-	 * in a trace session are using the same sink.  Therefore go through
-	 * the coresight bus and pick the first enabled sink.
-	 *
-	 * When operated from sysFS users are responsible to enable the sink
-	 * while from perf, the perf tools will do it based on the choice made
-	 * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
-	 */
-	sink = coresight_get_enabled_sink(true);
+	perf_event_drv_config_sync(event);
+
+	/* First get the sink to use for this event */
+	sink = etm_event_get_sink(event);
 	if (!sink)
 		goto err;
 
@@ -442,6 +492,57 @@ static void etm_addr_filters_sync(struct perf_event *event)
 	filters->nr_filters = i;
 }
 
+static const match_table_t config_tokens = {
+	{ ETM_CFG_SINK,		"%u.%s" },
+	{ ETM_CFG_NONE,		NULL },
+};
+
+static void *etm_drv_config_validate(struct perf_event *event, char *cstr)
+{
+	char *str, *to_parse, *sink = NULL;
+	int token, ret = -EINVAL;
+	substring_t args[MAX_OPT_ARGS];
+
+	to_parse = kstrdup(cstr, GFP_KERNEL);
+	if (!to_parse)
+		return ERR_PTR(-ENOMEM);
+
+	while ((str = strsep(&to_parse, " ,\n")) != NULL) {
+		if (!*str)
+			continue;
+
+		token = match_token(str, config_tokens, args);
+		switch (token) {
+		case ETM_CFG_SINK:
+			sink = kstrdup(str, GFP_KERNEL);
+			if (!sink) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			break;
+		default:
+			goto out;
+		}
+	}
+
+out:
+	kfree(to_parse);
+	return sink ? sink : ERR_PTR(ret);
+}
+
+static void etm_drv_config_sync(struct perf_event *event)
+{
+	struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
+
+	memcpy(event->hw.drv_config, drv_config->drv_config,
+	       CORESIGHT_DEVICE_MAX_NAME_LEN);
+}
+
+static void etm_drv_config_free(void *drv_data)
+{
+	kfree(drv_data);
+}
+
 int etm_perf_symlink(struct coresight_device *csdev, bool link)
 {
 	char entry[sizeof("cpu9999999")];
@@ -486,6 +587,9 @@ static int __init etm_perf_init(void)
 	etm_pmu.addr_filters_sync	= etm_addr_filters_sync;
 	etm_pmu.addr_filters_validate	= etm_addr_filters_validate;
 	etm_pmu.nr_addr_filters		= ETM_ADDR_CMP_MAX;
+	etm_pmu.drv_config_validate	= etm_drv_config_validate;
+	etm_pmu.drv_config_sync		= etm_drv_config_sync;
+	etm_pmu.drv_config_free		= etm_drv_config_free;
 
 	ret = perf_pmu_register(&etm_pmu, CORESIGHT_ETM_PMU_NAME, -1);
 	if (ret == 0)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 4197df4faf5e..175895c8d635 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -42,6 +42,16 @@ struct etm_filters {
 	bool			ssstatus;
 };
 
+enum etm_config_elem_type {
+	ETM_CFG_NONE = -1,
+	ETM_CFG_SINK,
+};
+
+struct etm_config_elem {
+	struct list_head		entry;
+	enum etm_config_elem_type	type;
+	void				*config;
+};
 
 #ifdef CONFIG_CORESIGHT
 int etm_perf_symlink(struct coresight_device *csdev, bool link);
-- 
2.7.4

Re: [PATCH 4/6] perf/aux: Make perf_event accessible to setup_aux()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 9745 bytes --]

Hi Mathieu,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.18-rc3 next-20180702]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mathieu-Poirier/perf-Add-ioctl-for-PMU-driver-configuration/20180703-064327
config: s390-defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=s390 

All error/warnings (new ones prefixed by >>):

>> arch/s390/kernel/perf_cpum_sf.c:1606:1: error: expected identifier or '(' before '{' token
    {
    ^
>> arch/s390/kernel/perf_cpum_sf.c:1604:14: warning: 'aux_buffer_setup' used but never defined
    static void *aux_buffer_setup(struct perf_event *event, void **pages,
                 ^~~~~~~~~~~~~~~~

vim +1606 arch/s390/kernel/perf_cpum_sf.c

ca5955cd Pu Hou          2016-11-11  1589  
ca5955cd Pu Hou          2016-11-11  1590  /*
ca5955cd Pu Hou          2016-11-11  1591   * aux_buffer_setup() - Setup AUX buffer for diagnostic mode sampling
ca5955cd Pu Hou          2016-11-11  1592   * @cpu:	On which to allocate, -1 means current
ca5955cd Pu Hou          2016-11-11  1593   * @pages:	Array of pointers to buffer pages passed from perf core
ca5955cd Pu Hou          2016-11-11  1594   * @nr_pages:	Total pages
ca5955cd Pu Hou          2016-11-11  1595   * @snapshot:	Flag for snapshot mode
ca5955cd Pu Hou          2016-11-11  1596   *
ca5955cd Pu Hou          2016-11-11  1597   * This is the callback when setup an event using AUX buffer. Perf tool can
ca5955cd Pu Hou          2016-11-11  1598   * trigger this by an additional mmap() call on the event. Unlike the buffer
ca5955cd Pu Hou          2016-11-11  1599   * for basic samples, AUX buffer belongs to the event. It is scheduled with
ca5955cd Pu Hou          2016-11-11  1600   * the task among online cpus when it is a per-thread event.
ca5955cd Pu Hou          2016-11-11  1601   *
ca5955cd Pu Hou          2016-11-11  1602   * Return the private AUX buffer structure if success or NULL if fails.
ca5955cd Pu Hou          2016-11-11  1603   */
ceb39bf0 Mathieu Poirier 2018-07-02 @1604  static void *aux_buffer_setup(struct perf_event *event, void **pages,
ceb39bf0 Mathieu Poirier 2018-07-02  1605  			      int nr_pages, bool snapshot);
ca5955cd Pu Hou          2016-11-11 @1606  {
ca5955cd Pu Hou          2016-11-11  1607  	struct sf_buffer *sfb;
ca5955cd Pu Hou          2016-11-11  1608  	struct aux_buffer *aux;
ca5955cd Pu Hou          2016-11-11  1609  	unsigned long *new, *tail;
ca5955cd Pu Hou          2016-11-11  1610  	int i, n_sdbt;
ca5955cd Pu Hou          2016-11-11  1611  
ca5955cd Pu Hou          2016-11-11  1612  	if (!nr_pages || !pages)
ca5955cd Pu Hou          2016-11-11  1613  		return NULL;
ca5955cd Pu Hou          2016-11-11  1614  
ca5955cd Pu Hou          2016-11-11  1615  	if (nr_pages > CPUM_SF_MAX_SDB * CPUM_SF_SDB_DIAG_FACTOR) {
ca5955cd Pu Hou          2016-11-11  1616  		pr_err("AUX buffer size (%i pages) is larger than the "
ca5955cd Pu Hou          2016-11-11  1617  		       "maximum sampling buffer limit\n",
ca5955cd Pu Hou          2016-11-11  1618  		       nr_pages);
ca5955cd Pu Hou          2016-11-11  1619  		return NULL;
ca5955cd Pu Hou          2016-11-11  1620  	} else if (nr_pages < CPUM_SF_MIN_SDB * CPUM_SF_SDB_DIAG_FACTOR) {
ca5955cd Pu Hou          2016-11-11  1621  		pr_err("AUX buffer size (%i pages) is less than the "
ca5955cd Pu Hou          2016-11-11  1622  		       "minimum sampling buffer limit\n",
ca5955cd Pu Hou          2016-11-11  1623  		       nr_pages);
ca5955cd Pu Hou          2016-11-11  1624  		return NULL;
ca5955cd Pu Hou          2016-11-11  1625  	}
ca5955cd Pu Hou          2016-11-11  1626  
ca5955cd Pu Hou          2016-11-11  1627  	/* Allocate aux_buffer struct for the event */
ca5955cd Pu Hou          2016-11-11  1628  	aux = kmalloc(sizeof(struct aux_buffer), GFP_KERNEL);
ca5955cd Pu Hou          2016-11-11  1629  	if (!aux)
ca5955cd Pu Hou          2016-11-11  1630  		goto no_aux;
ca5955cd Pu Hou          2016-11-11  1631  	sfb = &aux->sfb;
ca5955cd Pu Hou          2016-11-11  1632  
ca5955cd Pu Hou          2016-11-11  1633  	/* Allocate sdbt_index for fast reference */
ca5955cd Pu Hou          2016-11-11  1634  	n_sdbt = (nr_pages + CPUM_SF_SDB_PER_TABLE - 1) / CPUM_SF_SDB_PER_TABLE;
ca5955cd Pu Hou          2016-11-11  1635  	aux->sdbt_index = kmalloc_array(n_sdbt, sizeof(void *), GFP_KERNEL);
ca5955cd Pu Hou          2016-11-11  1636  	if (!aux->sdbt_index)
ca5955cd Pu Hou          2016-11-11  1637  		goto no_sdbt_index;
ca5955cd Pu Hou          2016-11-11  1638  
ca5955cd Pu Hou          2016-11-11  1639  	/* Allocate sdb_index for fast reference */
ca5955cd Pu Hou          2016-11-11  1640  	aux->sdb_index = kmalloc_array(nr_pages, sizeof(void *), GFP_KERNEL);
ca5955cd Pu Hou          2016-11-11  1641  	if (!aux->sdb_index)
ca5955cd Pu Hou          2016-11-11  1642  		goto no_sdb_index;
ca5955cd Pu Hou          2016-11-11  1643  
ca5955cd Pu Hou          2016-11-11  1644  	/* Allocate the first SDBT */
ca5955cd Pu Hou          2016-11-11  1645  	sfb->num_sdbt = 0;
ca5955cd Pu Hou          2016-11-11  1646  	sfb->sdbt = (unsigned long *) get_zeroed_page(GFP_KERNEL);
ca5955cd Pu Hou          2016-11-11  1647  	if (!sfb->sdbt)
ca5955cd Pu Hou          2016-11-11  1648  		goto no_sdbt;
ca5955cd Pu Hou          2016-11-11  1649  	aux->sdbt_index[sfb->num_sdbt++] = (unsigned long)sfb->sdbt;
ca5955cd Pu Hou          2016-11-11  1650  	tail = sfb->tail = sfb->sdbt;
ca5955cd Pu Hou          2016-11-11  1651  
ca5955cd Pu Hou          2016-11-11  1652  	/*
ca5955cd Pu Hou          2016-11-11  1653  	 * Link the provided pages of AUX buffer to SDBT.
ca5955cd Pu Hou          2016-11-11  1654  	 * Allocate SDBT if needed.
ca5955cd Pu Hou          2016-11-11  1655  	 */
ca5955cd Pu Hou          2016-11-11  1656  	for (i = 0; i < nr_pages; i++, tail++) {
ca5955cd Pu Hou          2016-11-11  1657  		if (require_table_link(tail)) {
ca5955cd Pu Hou          2016-11-11  1658  			new = (unsigned long *) get_zeroed_page(GFP_KERNEL);
ca5955cd Pu Hou          2016-11-11  1659  			if (!new)
ca5955cd Pu Hou          2016-11-11  1660  				goto no_sdbt;
ca5955cd Pu Hou          2016-11-11  1661  			aux->sdbt_index[sfb->num_sdbt++] = (unsigned long)new;
ca5955cd Pu Hou          2016-11-11  1662  			/* Link current page to tail of chain */
ca5955cd Pu Hou          2016-11-11  1663  			*tail = (unsigned long)(void *) new + 1;
ca5955cd Pu Hou          2016-11-11  1664  			tail = new;
ca5955cd Pu Hou          2016-11-11  1665  		}
ca5955cd Pu Hou          2016-11-11  1666  		/* Tail is the entry in a SDBT */
ca5955cd Pu Hou          2016-11-11  1667  		*tail = (unsigned long)pages[i];
ca5955cd Pu Hou          2016-11-11  1668  		aux->sdb_index[i] = (unsigned long)pages[i];
ca5955cd Pu Hou          2016-11-11  1669  	}
ca5955cd Pu Hou          2016-11-11  1670  	sfb->num_sdb = nr_pages;
ca5955cd Pu Hou          2016-11-11  1671  
ca5955cd Pu Hou          2016-11-11  1672  	/* Link the last entry in the SDBT to the first SDBT */
ca5955cd Pu Hou          2016-11-11  1673  	*tail = (unsigned long) sfb->sdbt + 1;
ca5955cd Pu Hou          2016-11-11  1674  	sfb->tail = tail;
ca5955cd Pu Hou          2016-11-11  1675  
ca5955cd Pu Hou          2016-11-11  1676  	/*
ca5955cd Pu Hou          2016-11-11  1677  	 * Initial all SDBs are zeroed. Mark it as empty.
ca5955cd Pu Hou          2016-11-11  1678  	 * So there is no need to clear the full indicator
ca5955cd Pu Hou          2016-11-11  1679  	 * when this event is first added.
ca5955cd Pu Hou          2016-11-11  1680  	 */
ca5955cd Pu Hou          2016-11-11  1681  	aux->empty_mark = sfb->num_sdb - 1;
ca5955cd Pu Hou          2016-11-11  1682  
ca5955cd Pu Hou          2016-11-11  1683  	debug_sprintf_event(sfdbg, 4, "aux_buffer_setup: setup %lu SDBTs"
ca5955cd Pu Hou          2016-11-11  1684  			    " and %lu SDBs\n",
ca5955cd Pu Hou          2016-11-11  1685  			    sfb->num_sdbt, sfb->num_sdb);
ca5955cd Pu Hou          2016-11-11  1686  
ca5955cd Pu Hou          2016-11-11  1687  	return aux;
ca5955cd Pu Hou          2016-11-11  1688  
ca5955cd Pu Hou          2016-11-11  1689  no_sdbt:
ca5955cd Pu Hou          2016-11-11  1690  	/* SDBs (AUX buffer pages) are freed by caller */
ca5955cd Pu Hou          2016-11-11  1691  	for (i = 0; i < sfb->num_sdbt; i++)
ca5955cd Pu Hou          2016-11-11  1692  		free_page(aux->sdbt_index[i]);
ca5955cd Pu Hou          2016-11-11  1693  	kfree(aux->sdb_index);
ca5955cd Pu Hou          2016-11-11  1694  no_sdb_index:
ca5955cd Pu Hou          2016-11-11  1695  	kfree(aux->sdbt_index);
ca5955cd Pu Hou          2016-11-11  1696  no_sdbt_index:
ca5955cd Pu Hou          2016-11-11  1697  	kfree(aux);
ca5955cd Pu Hou          2016-11-11  1698  no_aux:
ca5955cd Pu Hou          2016-11-11  1699  	return NULL;
ca5955cd Pu Hou          2016-11-11  1700  }
ca5955cd Pu Hou          2016-11-11  1701  

:::::: The code at line 1606 was first introduced by commit
:::::: ca5955cdeae744edd3dcc65d677e833fc29658c2 s390/cpumf: introduce AUX buffer for dump diagnostic sample data

:::::: TO: Pu Hou <bjhoupu@linux.vnet.ibm.com>
:::::: CC: Martin Schwidefsky <schwidefsky@de.ibm.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10840 bytes --]

Re: [PATCH 4/6] perf/aux: Make perf_event accessible to setup_aux()

[Hendrik Brueckner]

On Mon, Jul 02, 2018 at 04:33:28PM -0600, Mathieu Poirier wrote:
> It can be advantagous to have access to all the information conveyed by
> a perf_event when setting up the AUX buffer, as it is the case when
> dealing with PMU specific driver configuration communicated to the kernel
> using an ioctl() call.
> 
> As such simply replace the cpu information by the complete perf_event
> structure and change all affected customers.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  arch/s390/kernel/perf_cpum_sf.c                  | 4 ++--
>  arch/x86/events/intel/bts.c                      | 4 +++-
>  arch/x86/events/intel/pt.c                       | 5 +++--
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 6 +++---
>  drivers/perf/arm_spe_pmu.c                       | 6 +++---
>  include/linux/perf_event.h                       | 2 +-
>  kernel/events/ring_buffer.c                      | 2 +-
>  7 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
> index 0292d68e7dde..e06daaa08894 100644
> --- a/arch/s390/kernel/perf_cpum_sf.c
> +++ b/arch/s390/kernel/perf_cpum_sf.c
> @@ -1601,8 +1601,8 @@ static void aux_buffer_free(void *data)
>   *
>   * Return the private AUX buffer structure if success or NULL if fails.
>   */
> -static void *aux_buffer_setup(int cpu, void **pages, int nr_pages,
> -			      bool snapshot)
> +static void *aux_buffer_setup(struct perf_event *event, void **pages,
> +			      int nr_pages, bool snapshot);

Please remove the trailing semi-colon (;) in the function definition causing
the kbuild error.  Also, it would be great if you also could update the
function comment and replace the @cpu by the @event.

Many thanks.

Re: [PATCH 4/6] perf/aux: Make perf_event accessible to setup_aux()

[Alexander Shishkin]

On Mon, Jul 02, 2018 at 04:33:28PM -0600, Mathieu Poirier wrote:
> It can be advantagous to have access to all the information conveyed by
> a perf_event when setting up the AUX buffer, as it is the case when
> dealing with PMU specific driver configuration communicated to the kernel
> using an ioctl() call.
> 
> As such simply replace the cpu information by the complete perf_event
> structure and change all affected customers.

Ok, but can you provide details about which parts of perf_event you need
and how they are used? Is it the attribute? What's the ioctl command in
question? Also, should we worry that the PMU specific configuration isn't
part of the attribute? Or, if not, why? I think we talked about this
before, but I forgot and this patch needs to explain it anyway.

Thanks!
--
Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Alexander Shishkin]

On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
> This patch follows what has been done for filters by adding an ioctl()
> option to communicate to the kernel arbitrary PMU specific configuration
> that don't fit in the conventional struct perf_event_attr to the kernel.

Ok, so what *is* the PMU specific configuration that doesn't fit in the
attribute and needs to be re-configured by the driver using the generation
tracking?

> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  include/linux/perf_event.h |  54 ++++++++++++++++++++++
>  kernel/events/core.c       | 110 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 164 insertions(+)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 4d9c8f30ca6c..6e06b63c262f 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -178,6 +178,12 @@ struct hw_perf_event {
>  	/* Last sync'ed generation of filters */
>  	unsigned long			addr_filters_gen;
>  
> +	/* PMU driver configuration */
> +	void				*drv_config;
> +
> +	/* Last sync'ed generation of driver config */
> +	unsigned long			drv_config_gen;
> +
>  /*
>   * hw_perf_event::state flags; used to track the PERF_EF_* state.
>   */
> @@ -447,6 +453,26 @@ struct pmu {
>  	 * Filter events for PMU-specific reasons.
>  	 */
>  	int (*filter_match)		(struct perf_event *event); /* optional */
> +
> +	/*
> +	 * Valiate complex PMU configuration that don't fit in the
> +	 * perf_event_attr struct.  Returns a PMU specific pointer or an error
> +	 * value < 0.
> +	 *
> +	 * As with addr_filters_validate(), runs in the context of the ioctl()
> +	 * process and is not serialized with the rest of the PMU callbacks.

Yes, but what is it? I get it that it's probably in one of the other
patches, but we still need to mention it somewhere here.

> +	 */
> +	void *(*drv_config_validate)	(struct perf_event *event,
> +					 char *config_str);
> +
> +	/* Synchronize PMU driver configuration */
> +	void (*drv_config_sync)		(struct perf_event *event);
> +
> +	/*
> +	 * Release PMU specific configuration acquired by
> +	 * drv_config_validate()
> +	 */
> +	void (*drv_config_free)		(void *drv_data);
>  };
>  
>  enum perf_addr_filter_action_t {
> @@ -489,6 +515,11 @@ struct perf_addr_filters_head {
>  	unsigned int		nr_file_filters;
>  };
>  
> +struct perf_drv_config {
> +	void		*drv_config;
> +	raw_spinlock_t	lock;
> +};
> +
>  /**
>   * enum perf_event_state - the states of a event
>   */
> @@ -668,6 +699,10 @@ struct perf_event {
>  	unsigned long			*addr_filters_offs;
>  	unsigned long			addr_filters_gen;
>  
> +	/* PMU driver specific configuration */
> +	struct perf_drv_config		drv_config;
> +	unsigned long			drv_config_gen;
> +
>  	void (*destroy)(struct perf_event *);
>  	struct rcu_head			rcu_head;
>  
> @@ -1234,6 +1269,13 @@ static inline bool has_addr_filter(struct perf_event *event)
>  	return event->pmu->nr_addr_filters;
>  }
>  
> +static inline bool has_drv_config(struct perf_event *event)
> +{
> +	return event->pmu->drv_config_validate &&
> +	       event->pmu->drv_config_sync &&
> +	       event->pmu->drv_config_free;
> +}
> +
>  /*
>   * An inherited event uses parent's filters
>   */
> @@ -1248,7 +1290,19 @@ perf_event_addr_filters(struct perf_event *event)
>  	return ifh;
>  }
>  
> +static inline struct perf_drv_config *
> +perf_event_get_drv_config(struct perf_event *event)
> +{
> +	struct perf_drv_config *cfg = &event->drv_config;
> +
> +	if (event->parent)
> +		cfg = &event->parent->drv_config;
> +
> +	return cfg;
> +}
> +
>  extern void perf_event_addr_filters_sync(struct perf_event *event);
> +extern void perf_event_drv_config_sync(struct perf_event *event);
>  
>  extern int perf_output_begin(struct perf_output_handle *handle,
>  			     struct perf_event *event, unsigned int size);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 8f0434a9951a..701839866789 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2829,6 +2829,29 @@ void perf_event_addr_filters_sync(struct perf_event *event)
>  }
>  EXPORT_SYMBOL_GPL(perf_event_addr_filters_sync);
>  
> +/*
> + * PMU driver configuration works the same way as filter management above,
> + * but without the need to deal with memory mapping.  Driver configuration
> + * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
> + * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
> + * bring the configuration information within reach of the PMU.

Wait a second. The reason why we dance around with the generations of filters
is the locking order of ctx::mutex vs mmap_sem. In an mmap path, where we're
notified about mapping changes, we're called under the latter, and we'd need
to grab the former to update the event configuration. In your case, the
update comes in via perf_ioctl(), where we're already holding the ctx::mutex,
so you can just kick the PMU right there, via an event_function_call() or
perf_event_stop(restart=1). In the latter case, your pmu::start() would just
grab the new configuration. Should also be about 90% less code. :)

Would this work for you or am I misunderstanding something about your
requirements?

> + */
> +void perf_event_drv_config_sync(struct perf_event *event)
> +{
> +	struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
> +
> +	if (!has_drv_config(event))
> +		return;
> +
> +	raw_spin_lock(&drv_config->lock);
> +	if (event->drv_config_gen != event->hw.drv_config_gen) {
> +		event->pmu->drv_config_sync(event);
> +		event->hw.drv_config_gen = event->drv_config_gen;
> +	}
> +	raw_spin_unlock(&drv_config->lock);
> +}
> +EXPORT_SYMBOL_GPL(perf_event_drv_config_sync);
> +
>  static int _perf_event_refresh(struct perf_event *event, int refresh)
>  {
>  	/*
> @@ -4410,6 +4433,7 @@ static bool exclusive_event_installable(struct perf_event *event,
>  
>  static void perf_addr_filters_splice(struct perf_event *event,
>  				       struct list_head *head);
> +static void perf_drv_config_splice(struct perf_event *event, void *drv_data);
>  
>  static void _free_event(struct perf_event *event)
>  {
> @@ -4440,6 +4464,7 @@ static void _free_event(struct perf_event *event)
>  	perf_event_free_bpf_prog(event);
>  	perf_addr_filters_splice(event, NULL);
>  	kfree(event->addr_filters_offs);
> +	perf_drv_config_splice(event, NULL);
>  
>  	if (event->destroy)
>  		event->destroy(event);
> @@ -5002,6 +5027,8 @@ static inline int perf_fget_light(int fd, struct fd *p)
>  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_drv_config(struct perf_event *event,
> +				     void __user *arg);
>  static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
>  static int perf_copy_attr(struct perf_event_attr __user *uattr,
>  			  struct perf_event_attr *attr);
> @@ -5088,6 +5115,10 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
>  
>  		return perf_event_modify_attr(event,  &new_attr);
>  	}
> +
> +	case PERF_EVENT_IOC_SET_DRV_CONFIG:
> +		return perf_event_set_drv_config(event, (void __user *)arg);
> +
>  	default:
>  		return -ENOTTY;
>  	}
> @@ -9086,6 +9117,85 @@ static int perf_event_set_filter(struct perf_event *event, void __user *arg)
>  	return ret;
>  }
>  
> +static void perf_drv_config_splice(struct perf_event *event, void *drv_data)

I think the address filter counterpart is called "splice" because it takes
a list_head as a parameter and splices that list into the list of filters.
I'd suggest that this one is more like "replace", but up to you.

> +{
> +	unsigned long flags;
> +	void *old_drv_data;
> +
> +	if (!has_drv_config(event))
> +		return;
> +
> +	/* Children take their configuration from their parent */
> +	if (event->parent)
> +		return;
> +
> +	raw_spin_lock_irqsave(&event->drv_config.lock, flags);
> +
> +	old_drv_data = event->drv_config.drv_config;
> +	event->drv_config.drv_config = drv_data;

Now I'm thinking: should we reset the generation here (and also in the
address filters bit)? At least, it deserves a comment.

> +
> +	raw_spin_unlock_irqrestore(&event->drv_config.lock, flags);
> +
> +	event->pmu->drv_config_free(old_drv_data);
> +}
> +
> +static void perf_event_drv_config_apply(struct perf_event *event)
> +{
> +	unsigned long flags;
> +	struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
> +
> +	/* Notify event that a new configuration is available */
> +	raw_spin_lock_irqsave(&drv_config->lock, flags);
> +	event->drv_config_gen++;
> +	raw_spin_unlock_irqrestore(&drv_config->lock, flags);

Should we also mention how this new locks fits into the existing locking
order?

Regards,
--
Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Alexander Shishkin]

On Tue, Jul 03, 2018 at 01:03:48PM +0300, Alexander Shishkin wrote:
> On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
> > +/*
> > + * PMU driver configuration works the same way as filter management above,
> > + * but without the need to deal with memory mapping.  Driver configuration
> > + * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
> > + * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
> > + * bring the configuration information within reach of the PMU.
> 
> Wait a second. The reason why we dance around with the generations of filters
> is the locking order of ctx::mutex vs mmap_sem. In an mmap path, where we're
> notified about mapping changes, we're called under the latter, and we'd need
> to grab the former to update the event configuration. In your case, the
> update comes in via perf_ioctl(), where we're already holding the ctx::mutex,
> so you can just kick the PMU right there, via an event_function_call() or
> perf_event_stop(restart=1). In the latter case, your pmu::start() would just
> grab the new configuration. Should also be about 90% less code. :)

Also, since it affects the AUX buffer configuration, it is probably a one
time ioctl command that you issue before you mmap the buffer. If that's
the case, you don't even have to worry about stopping the event, as it
shouldn't be running, because without the buffer perf_aux_output_begin()
should fail and so should the pmu::add() iirc.

Regards,
--
Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Jiri Olsa]

On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:

SNIP

> +static int
> +perf_event_process_drv_config(struct perf_event *event, char *config_str)
> +{
> +	int ret = -EINVAL;
> +	void *drv_data;
> +
> +	/* Make sure ctx.mutex it held */
> +	lockdep_assert_held(&event->ctx->mutex);
> +
> +	/* Children take their configuration from their parent */
> +	if (WARN_ON_ONCE(event->parent))
> +		goto out;
> +
> +	drv_data = event->pmu->drv_config_validate(event, config_str);
> +	if (!drv_data)
> +		goto out;

what's this check for? doc does not mention that,
also the coresight callback does not return NULL

jirka

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Jiri Olsa]

On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:

SNIP

> +	ret = 0;
> +out:
> +	return ret;
> +}
> +
> +static int perf_event_set_drv_config(struct perf_event *event, void __user *arg)
> +{
> +	int ret = -EINVAL;
> +	char *config_str;
> +
> +	config_str = strndup_user(arg, PAGE_SIZE);
> +	if (IS_ERR(config_str))
> +		return PTR_ERR(config_str);
> +
> +	if (has_drv_config(event))

would it be better to make this check before
the alloc/copy data from user takes place?

jirka

Re: [PATCH 4/6] perf/aux: Make perf_event accessible to setup_aux()

[Mathieu Poirier]

On Tue, 3 Jul 2018 at 01:31, Hendrik Brueckner <brueckner@linux.ibm.com> wrote:
>
> On Mon, Jul 02, 2018 at 04:33:28PM -0600, Mathieu Poirier wrote:
> > It can be advantagous to have access to all the information conveyed by
> > a perf_event when setting up the AUX buffer, as it is the case when
> > dealing with PMU specific driver configuration communicated to the kernel
> > using an ioctl() call.
> >
> > As such simply replace the cpu information by the complete perf_event
> > structure and change all affected customers.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  arch/s390/kernel/perf_cpum_sf.c                  | 4 ++--
> >  arch/x86/events/intel/bts.c                      | 4 +++-
> >  arch/x86/events/intel/pt.c                       | 5 +++--
> >  drivers/hwtracing/coresight/coresight-etm-perf.c | 6 +++---
> >  drivers/perf/arm_spe_pmu.c                       | 6 +++---
> >  include/linux/perf_event.h                       | 2 +-
> >  kernel/events/ring_buffer.c                      | 2 +-
> >  7 files changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
> > index 0292d68e7dde..e06daaa08894 100644
> > --- a/arch/s390/kernel/perf_cpum_sf.c
> > +++ b/arch/s390/kernel/perf_cpum_sf.c
> > @@ -1601,8 +1601,8 @@ static void aux_buffer_free(void *data)
> >   *
> >   * Return the private AUX buffer structure if success or NULL if fails.
> >   */
> > -static void *aux_buffer_setup(int cpu, void **pages, int nr_pages,
> > -                           bool snapshot)
> > +static void *aux_buffer_setup(struct perf_event *event, void **pages,
> > +                           int nr_pages, bool snapshot);
>
> Please remove the trailing semi-colon (;) in the function definition causing
> the kbuild error.  Also, it would be great if you also could update the
> function comment and replace the @cpu by the @event.

Had I realised it was just as easy to compile for s390 as it is for
ARM64, I would have done so ahead of time.  Well done on that front.
All fixed now.

Thanks,
Mathieu

>
> Many thanks.
>

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

On Tue, 3 Jul 2018 at 07:40, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
>
> SNIP
>
> > +static int
> > +perf_event_process_drv_config(struct perf_event *event, char *config_str)
> > +{
> > +     int ret = -EINVAL;
> > +     void *drv_data;
> > +
> > +     /* Make sure ctx.mutex it held */
> > +     lockdep_assert_held(&event->ctx->mutex);
> > +
> > +     /* Children take their configuration from their parent */
> > +     if (WARN_ON_ONCE(event->parent))
> > +             goto out;
> > +
> > +     drv_data = event->pmu->drv_config_validate(event, config_str);
> > +     if (!drv_data)
> > +             goto out;
>
> what's this check for? doc does not mention that,
> also the coresight callback does not return NULL
>

Very true - thanks for pointing this out.

Mathieu

> jirka

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

Hi Alex,

On Tue, 3 Jul 2018 at 04:03, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
>
> On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
> > This patch follows what has been done for filters by adding an ioctl()
> > option to communicate to the kernel arbitrary PMU specific configuration
> > that don't fit in the conventional struct perf_event_attr to the kernel.
>
> Ok, so what *is* the PMU specific configuration that doesn't fit in the
> attribute and needs to be re-configured by the driver using the generation
> tracking?
>

In this patchset I'm am after the specification of sink information
for each event, i.e what sink a CPU is supposed to use for the
session.  I simply don't see putting something that PMU specific in
the generic perf_event_attr structure.  I also intend to use the same
ioctl mechanism to communicate complex tracer configuration for
sequencers, counters and input events.  I don't see a nice way of
doing that from the perf_event_attr, and that is even without thinking
about the different flavours of tracers out there, all with their own
features.

I've looked around and the only clean way I found to support this is
via an ioctl().  That way each tracer can easily identify the sink it
should be using without smearing the perf_event_attr structure.  I
would be happy to explore a different avenue should you think of
something.

Thanks,
Mathieu

> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  include/linux/perf_event.h |  54 ++++++++++++++++++++++
> >  kernel/events/core.c       | 110 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 164 insertions(+)
> >
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index 4d9c8f30ca6c..6e06b63c262f 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -178,6 +178,12 @@ struct hw_perf_event {
> >       /* Last sync'ed generation of filters */
> >       unsigned long                   addr_filters_gen;
> >
> > +     /* PMU driver configuration */
> > +     void                            *drv_config;
> > +
> > +     /* Last sync'ed generation of driver config */
> > +     unsigned long                   drv_config_gen;
> > +
> >  /*
> >   * hw_perf_event::state flags; used to track the PERF_EF_* state.
> >   */
> > @@ -447,6 +453,26 @@ struct pmu {
> >        * Filter events for PMU-specific reasons.
> >        */
> >       int (*filter_match)             (struct perf_event *event); /* optional */
> > +
> > +     /*
> > +      * Valiate complex PMU configuration that don't fit in the
> > +      * perf_event_attr struct.  Returns a PMU specific pointer or an error
> > +      * value < 0.
> > +      *
> > +      * As with addr_filters_validate(), runs in the context of the ioctl()
> > +      * process and is not serialized with the rest of the PMU callbacks.
>
> Yes, but what is it? I get it that it's probably in one of the other
> patches, but we still need to mention it somewhere here.
>
> > +      */
> > +     void *(*drv_config_validate)    (struct perf_event *event,
> > +                                      char *config_str);
> > +
> > +     /* Synchronize PMU driver configuration */
> > +     void (*drv_config_sync)         (struct perf_event *event);
> > +
> > +     /*
> > +      * Release PMU specific configuration acquired by
> > +      * drv_config_validate()
> > +      */
> > +     void (*drv_config_free)         (void *drv_data);
> >  };
> >
> >  enum perf_addr_filter_action_t {
> > @@ -489,6 +515,11 @@ struct perf_addr_filters_head {
> >       unsigned int            nr_file_filters;
> >  };
> >
> > +struct perf_drv_config {
> > +     void            *drv_config;
> > +     raw_spinlock_t  lock;
> > +};
> > +
> >  /**
> >   * enum perf_event_state - the states of a event
> >   */
> > @@ -668,6 +699,10 @@ struct perf_event {
> >       unsigned long                   *addr_filters_offs;
> >       unsigned long                   addr_filters_gen;
> >
> > +     /* PMU driver specific configuration */
> > +     struct perf_drv_config          drv_config;
> > +     unsigned long                   drv_config_gen;
> > +
> >       void (*destroy)(struct perf_event *);
> >       struct rcu_head                 rcu_head;
> >
> > @@ -1234,6 +1269,13 @@ static inline bool has_addr_filter(struct perf_event *event)
> >       return event->pmu->nr_addr_filters;
> >  }
> >
> > +static inline bool has_drv_config(struct perf_event *event)
> > +{
> > +     return event->pmu->drv_config_validate &&
> > +            event->pmu->drv_config_sync &&
> > +            event->pmu->drv_config_free;
> > +}
> > +
> >  /*
> >   * An inherited event uses parent's filters
> >   */
> > @@ -1248,7 +1290,19 @@ perf_event_addr_filters(struct perf_event *event)
> >       return ifh;
> >  }
> >
> > +static inline struct perf_drv_config *
> > +perf_event_get_drv_config(struct perf_event *event)
> > +{
> > +     struct perf_drv_config *cfg = &event->drv_config;
> > +
> > +     if (event->parent)
> > +             cfg = &event->parent->drv_config;
> > +
> > +     return cfg;
> > +}
> > +
> >  extern void perf_event_addr_filters_sync(struct perf_event *event);
> > +extern void perf_event_drv_config_sync(struct perf_event *event);
> >
> >  extern int perf_output_begin(struct perf_output_handle *handle,
> >                            struct perf_event *event, unsigned int size);
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 8f0434a9951a..701839866789 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -2829,6 +2829,29 @@ void perf_event_addr_filters_sync(struct perf_event *event)
> >  }
> >  EXPORT_SYMBOL_GPL(perf_event_addr_filters_sync);
> >
> > +/*
> > + * PMU driver configuration works the same way as filter management above,
> > + * but without the need to deal with memory mapping.  Driver configuration
> > + * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
> > + * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
> > + * bring the configuration information within reach of the PMU.
>
> Wait a second. The reason why we dance around with the generations of filters
> is the locking order of ctx::mutex vs mmap_sem. In an mmap path, where we're
> notified about mapping changes, we're called under the latter, and we'd need
> to grab the former to update the event configuration. In your case, the
> update comes in via perf_ioctl(), where we're already holding the ctx::mutex,
> so you can just kick the PMU right there, via an event_function_call() or
> perf_event_stop(restart=1). In the latter case, your pmu::start() would just
> grab the new configuration. Should also be about 90% less code. :)
>
> Would this work for you or am I misunderstanding something about your
> requirements?
>
> > + */
> > +void perf_event_drv_config_sync(struct perf_event *event)
> > +{
> > +     struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
> > +
> > +     if (!has_drv_config(event))
> > +             return;
> > +
> > +     raw_spin_lock(&drv_config->lock);
> > +     if (event->drv_config_gen != event->hw.drv_config_gen) {
> > +             event->pmu->drv_config_sync(event);
> > +             event->hw.drv_config_gen = event->drv_config_gen;
> > +     }
> > +     raw_spin_unlock(&drv_config->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(perf_event_drv_config_sync);
> > +
> >  static int _perf_event_refresh(struct perf_event *event, int refresh)
> >  {
> >       /*
> > @@ -4410,6 +4433,7 @@ static bool exclusive_event_installable(struct perf_event *event,
> >
> >  static void perf_addr_filters_splice(struct perf_event *event,
> >                                      struct list_head *head);
> > +static void perf_drv_config_splice(struct perf_event *event, void *drv_data);
> >
> >  static void _free_event(struct perf_event *event)
> >  {
> > @@ -4440,6 +4464,7 @@ static void _free_event(struct perf_event *event)
> >       perf_event_free_bpf_prog(event);
> >       perf_addr_filters_splice(event, NULL);
> >       kfree(event->addr_filters_offs);
> > +     perf_drv_config_splice(event, NULL);
> >
> >       if (event->destroy)
> >               event->destroy(event);
> > @@ -5002,6 +5027,8 @@ static inline int perf_fget_light(int fd, struct fd *p)
> >  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_drv_config(struct perf_event *event,
> > +                                  void __user *arg);
> >  static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
> >  static int perf_copy_attr(struct perf_event_attr __user *uattr,
> >                         struct perf_event_attr *attr);
> > @@ -5088,6 +5115,10 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
> >
> >               return perf_event_modify_attr(event,  &new_attr);
> >       }
> > +
> > +     case PERF_EVENT_IOC_SET_DRV_CONFIG:
> > +             return perf_event_set_drv_config(event, (void __user *)arg);
> > +
> >       default:
> >               return -ENOTTY;
> >       }
> > @@ -9086,6 +9117,85 @@ static int perf_event_set_filter(struct perf_event *event, void __user *arg)
> >       return ret;
> >  }
> >
> > +static void perf_drv_config_splice(struct perf_event *event, void *drv_data)
>
> I think the address filter counterpart is called "splice" because it takes
> a list_head as a parameter and splices that list into the list of filters.
> I'd suggest that this one is more like "replace", but up to you.
>
> > +{
> > +     unsigned long flags;
> > +     void *old_drv_data;
> > +
> > +     if (!has_drv_config(event))
> > +             return;
> > +
> > +     /* Children take their configuration from their parent */
> > +     if (event->parent)
> > +             return;
> > +
> > +     raw_spin_lock_irqsave(&event->drv_config.lock, flags);
> > +
> > +     old_drv_data = event->drv_config.drv_config;
> > +     event->drv_config.drv_config = drv_data;
>
> Now I'm thinking: should we reset the generation here (and also in the
> address filters bit)? At least, it deserves a comment.
>
> > +
> > +     raw_spin_unlock_irqrestore(&event->drv_config.lock, flags);
> > +
> > +     event->pmu->drv_config_free(old_drv_data);
> > +}
> > +
> > +static void perf_event_drv_config_apply(struct perf_event *event)
> > +{
> > +     unsigned long flags;
> > +     struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
> > +
> > +     /* Notify event that a new configuration is available */
> > +     raw_spin_lock_irqsave(&drv_config->lock, flags);
> > +     event->drv_config_gen++;
> > +     raw_spin_unlock_irqrestore(&drv_config->lock, flags);
>
> Should we also mention how this new locks fits into the existing locking
> order?
>
> Regards,
> --
> Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

On Tue, 3 Jul 2018 at 04:57, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
>
> On Tue, Jul 03, 2018 at 01:03:48PM +0300, Alexander Shishkin wrote:
> > On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
> > > +/*
> > > + * PMU driver configuration works the same way as filter management above,
> > > + * but without the need to deal with memory mapping.  Driver configuration
> > > + * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
> > > + * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
> > > + * bring the configuration information within reach of the PMU.
> >
> > Wait a second. The reason why we dance around with the generations of filters
> > is the locking order of ctx::mutex vs mmap_sem. In an mmap path, where we're
> > notified about mapping changes, we're called under the latter, and we'd need
> > to grab the former to update the event configuration. In your case, the
> > update comes in via perf_ioctl(), where we're already holding the ctx::mutex,
> > so you can just kick the PMU right there, via an event_function_call() or
> > perf_event_stop(restart=1). In the latter case, your pmu::start() would just
> > grab the new configuration. Should also be about 90% less code. :)
>
> Also, since it affects the AUX buffer configuration, it is probably a one
> time ioctl command that you issue before you mmap the buffer. If that's
> the case, you don't even have to worry about stopping the event, as it
> shouldn't be running, because without the buffer perf_aux_output_begin()
> should fail and so should the pmu::add() iirc.
>

The idea behind the current approach was to make the SET_DRV_CONFIG
ioctl() usable by other drivers where multiple ioctl() calls could be
performed while a session in ongoing.  I also opted to introduce a
_sync() function to let the PMU refresh its configuration at the time
of its own choosing rather than having to interrupt the session.

But all I need for coresight is to have available the sink information
and PMU configuration (in an upcoming patchset) by the time
setup_aux() is called.  You are correct, this is a one time
configuration and since the event isn't running there is no need for
locking - I should be able to access the PMU when the ioctl is called.

If you are fine with this bare-bone scenario and don't care much about
usability in different situation, I'll do a respin with minimal
functionality that cover my needs.

Thanks for the review,
Mathieu

> Regards,
> --
> Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

On Tue, 3 Jul 2018 at 04:03, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
>
> On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
> > This patch follows what has been done for filters by adding an ioctl()
> > option to communicate to the kernel arbitrary PMU specific configuration
> > that don't fit in the conventional struct perf_event_attr to the kernel.
>
> Ok, so what *is* the PMU specific configuration that doesn't fit in the
> attribute and needs to be re-configured by the driver using the generation
> tracking?
>
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  include/linux/perf_event.h |  54 ++++++++++++++++++++++
> >  kernel/events/core.c       | 110 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 164 insertions(+)
> >
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index 4d9c8f30ca6c..6e06b63c262f 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -178,6 +178,12 @@ struct hw_perf_event {
> >       /* Last sync'ed generation of filters */
> >       unsigned long                   addr_filters_gen;
> >
> > +     /* PMU driver configuration */
> > +     void                            *drv_config;
> > +
> > +     /* Last sync'ed generation of driver config */
> > +     unsigned long                   drv_config_gen;
> > +
> >  /*
> >   * hw_perf_event::state flags; used to track the PERF_EF_* state.
> >   */
> > @@ -447,6 +453,26 @@ struct pmu {
> >        * Filter events for PMU-specific reasons.
> >        */
> >       int (*filter_match)             (struct perf_event *event); /* optional */
> > +
> > +     /*
> > +      * Valiate complex PMU configuration that don't fit in the
> > +      * perf_event_attr struct.  Returns a PMU specific pointer or an error
> > +      * value < 0.
> > +      *
> > +      * As with addr_filters_validate(), runs in the context of the ioctl()
> > +      * process and is not serialized with the rest of the PMU callbacks.
>
> Yes, but what is it? I get it that it's probably in one of the other
> patches, but we still need to mention it somewhere here.

I could write a more detailed description here, something about the
specification of sink and configuration of coresight specific features
(sequencers, counters, input/output) but I decided to keep things
generic.  Rather than doing that I thought it best to leave things
generic and let people look at the code for more details should they
want to.  Let me know what you think is best.

>
> > +      */
> > +     void *(*drv_config_validate)    (struct perf_event *event,
> > +                                      char *config_str);
> > +
> > +     /* Synchronize PMU driver configuration */
> > +     void (*drv_config_sync)         (struct perf_event *event);
> > +
> > +     /*
> > +      * Release PMU specific configuration acquired by
> > +      * drv_config_validate()
> > +      */
> > +     void (*drv_config_free)         (void *drv_data);
> >  };
> >
> >  enum perf_addr_filter_action_t {
> > @@ -489,6 +515,11 @@ struct perf_addr_filters_head {
> >       unsigned int            nr_file_filters;
> >  };
> >
> > +struct perf_drv_config {
> > +     void            *drv_config;
> > +     raw_spinlock_t  lock;
> > +};
> > +
> >  /**
> >   * enum perf_event_state - the states of a event
> >   */
> > @@ -668,6 +699,10 @@ struct perf_event {
> >       unsigned long                   *addr_filters_offs;
> >       unsigned long                   addr_filters_gen;
> >
> > +     /* PMU driver specific configuration */
> > +     struct perf_drv_config          drv_config;
> > +     unsigned long                   drv_config_gen;
> > +
> >       void (*destroy)(struct perf_event *);
> >       struct rcu_head                 rcu_head;
> >
> > @@ -1234,6 +1269,13 @@ static inline bool has_addr_filter(struct perf_event *event)
> >       return event->pmu->nr_addr_filters;
> >  }
> >
> > +static inline bool has_drv_config(struct perf_event *event)
> > +{
> > +     return event->pmu->drv_config_validate &&
> > +            event->pmu->drv_config_sync &&
> > +            event->pmu->drv_config_free;
> > +}
> > +
> >  /*
> >   * An inherited event uses parent's filters
> >   */
> > @@ -1248,7 +1290,19 @@ perf_event_addr_filters(struct perf_event *event)
> >       return ifh;
> >  }
> >
> > +static inline struct perf_drv_config *
> > +perf_event_get_drv_config(struct perf_event *event)
> > +{
> > +     struct perf_drv_config *cfg = &event->drv_config;
> > +
> > +     if (event->parent)
> > +             cfg = &event->parent->drv_config;
> > +
> > +     return cfg;
> > +}
> > +
> >  extern void perf_event_addr_filters_sync(struct perf_event *event);
> > +extern void perf_event_drv_config_sync(struct perf_event *event);
> >
> >  extern int perf_output_begin(struct perf_output_handle *handle,
> >                            struct perf_event *event, unsigned int size);
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 8f0434a9951a..701839866789 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -2829,6 +2829,29 @@ void perf_event_addr_filters_sync(struct perf_event *event)
> >  }
> >  EXPORT_SYMBOL_GPL(perf_event_addr_filters_sync);
> >
> > +/*
> > + * PMU driver configuration works the same way as filter management above,
> > + * but without the need to deal with memory mapping.  Driver configuration
> > + * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
> > + * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
> > + * bring the configuration information within reach of the PMU.
>
> Wait a second. The reason why we dance around with the generations of filters
> is the locking order of ctx::mutex vs mmap_sem. In an mmap path, where we're
> notified about mapping changes, we're called under the latter, and we'd need
> to grab the former to update the event configuration. In your case, the
> update comes in via perf_ioctl(), where we're already holding the ctx::mutex,
> so you can just kick the PMU right there, via an event_function_call() or
> perf_event_stop(restart=1). In the latter case, your pmu::start() would just
> grab the new configuration. Should also be about 90% less code. :)
>
> Would this work for you or am I misunderstanding something about your
> requirements?
>
> > + */
> > +void perf_event_drv_config_sync(struct perf_event *event)
> > +{
> > +     struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
> > +
> > +     if (!has_drv_config(event))
> > +             return;
> > +
> > +     raw_spin_lock(&drv_config->lock);
> > +     if (event->drv_config_gen != event->hw.drv_config_gen) {
> > +             event->pmu->drv_config_sync(event);
> > +             event->hw.drv_config_gen = event->drv_config_gen;
> > +     }
> > +     raw_spin_unlock(&drv_config->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(perf_event_drv_config_sync);
> > +
> >  static int _perf_event_refresh(struct perf_event *event, int refresh)
> >  {
> >       /*
> > @@ -4410,6 +4433,7 @@ static bool exclusive_event_installable(struct perf_event *event,
> >
> >  static void perf_addr_filters_splice(struct perf_event *event,
> >                                      struct list_head *head);
> > +static void perf_drv_config_splice(struct perf_event *event, void *drv_data);
> >
> >  static void _free_event(struct perf_event *event)
> >  {
> > @@ -4440,6 +4464,7 @@ static void _free_event(struct perf_event *event)
> >       perf_event_free_bpf_prog(event);
> >       perf_addr_filters_splice(event, NULL);
> >       kfree(event->addr_filters_offs);
> > +     perf_drv_config_splice(event, NULL);
> >
> >       if (event->destroy)
> >               event->destroy(event);
> > @@ -5002,6 +5027,8 @@ static inline int perf_fget_light(int fd, struct fd *p)
> >  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_drv_config(struct perf_event *event,
> > +                                  void __user *arg);
> >  static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
> >  static int perf_copy_attr(struct perf_event_attr __user *uattr,
> >                         struct perf_event_attr *attr);
> > @@ -5088,6 +5115,10 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
> >
> >               return perf_event_modify_attr(event,  &new_attr);
> >       }
> > +
> > +     case PERF_EVENT_IOC_SET_DRV_CONFIG:
> > +             return perf_event_set_drv_config(event, (void __user *)arg);
> > +
> >       default:
> >               return -ENOTTY;
> >       }
> > @@ -9086,6 +9117,85 @@ static int perf_event_set_filter(struct perf_event *event, void __user *arg)
> >       return ret;
> >  }
> >
> > +static void perf_drv_config_splice(struct perf_event *event, void *drv_data)
>
> I think the address filter counterpart is called "splice" because it takes
> a list_head as a parameter and splices that list into the list of filters.
> I'd suggest that this one is more like "replace", but up to you.

I was on the fence about the naming convention...  I wanted the
drv_config mechanism to be as close as possible to the filters, that
way if someone understands the filters they'll easily understand the
drv_config.  But it may be more confusing than anything else - I'll
change it.

>
> > +{
> > +     unsigned long flags;
> > +     void *old_drv_data;
> > +
> > +     if (!has_drv_config(event))
> > +             return;
> > +
> > +     /* Children take their configuration from their parent */
> > +     if (event->parent)
> > +             return;
> > +
> > +     raw_spin_lock_irqsave(&event->drv_config.lock, flags);
> > +
> > +     old_drv_data = event->drv_config.drv_config;
> > +     event->drv_config.drv_config = drv_data;
>
> Now I'm thinking: should we reset the generation here (and also in the
> address filters bit)? At least, it deserves a comment.

I thought your way of doing things was quite nice, hence doing the
same...  xyz_splice() does exactly that, it replaces the value but
downstream won't see it for as long as xyz_gen isn't updated - and
that is exactly what xyz_apply() does.  I can add a comment to clarify
how things work but I think we should keep the current scheme.

>
> > +
> > +     raw_spin_unlock_irqrestore(&event->drv_config.lock, flags);
> > +
> > +     event->pmu->drv_config_free(old_drv_data);
> > +}
> > +
> > +static void perf_event_drv_config_apply(struct perf_event *event)
> > +{
> > +     unsigned long flags;
> > +     struct perf_drv_config *drv_config = perf_event_get_drv_config(event);
> > +
> > +     /* Notify event that a new configuration is available */
> > +     raw_spin_lock_irqsave(&drv_config->lock, flags);
> > +     event->drv_config_gen++;
> > +     raw_spin_unlock_irqrestore(&drv_config->lock, flags);
>
> Should we also mention how this new locks fits into the existing locking
> order?
>
> Regards,
> --
> Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On Tue, 3 Jul 2018 at 04:57, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>>
>> On Tue, Jul 03, 2018 at 01:03:48PM +0300, Alexander Shishkin wrote:
>> > On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
>> > > +/*
>> > > + * PMU driver configuration works the same way as filter management above,
>> > > + * but without the need to deal with memory mapping.  Driver configuration
>> > > + * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
>> > > + * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
>> > > + * bring the configuration information within reach of the PMU.
>> >
>> > Wait a second. The reason why we dance around with the generations of filters
>> > is the locking order of ctx::mutex vs mmap_sem. In an mmap path, where we're
>> > notified about mapping changes, we're called under the latter, and we'd need
>> > to grab the former to update the event configuration. In your case, the
>> > update comes in via perf_ioctl(), where we're already holding the ctx::mutex,
>> > so you can just kick the PMU right there, via an event_function_call() or
>> > perf_event_stop(restart=1). In the latter case, your pmu::start() would just
>> > grab the new configuration. Should also be about 90% less code. :)
>>
>> Also, since it affects the AUX buffer configuration, it is probably a one
>> time ioctl command that you issue before you mmap the buffer. If that's
>> the case, you don't even have to worry about stopping the event, as it
>> shouldn't be running, because without the buffer perf_aux_output_begin()
>> should fail and so should the pmu::add() iirc.
>>
>
> The idea behind the current approach was to make the SET_DRV_CONFIG
> ioctl() usable by other drivers where multiple ioctl() calls could be
> performed while a session in ongoing.  I also opted to introduce a
> _sync() function to let the PMU refresh its configuration at the time
> of its own choosing rather than having to interrupt the session.

Yes, but the times of PMU's own choosing would still be more or less
limited to ->start()/->stop().

You can also do an event_function_call(), which would call
->config_sync(), which would be free to decide what to do with the new
information, up to and including doing a ->stop()/->start() sequence. My
guess is that you'd want to do either of the following:
 * decide to apply the new configuration immediately, and do the
 start-stop thing,
 * decide to defer the new configuration until the next ->start().

Both should work via cross call directly from the ioctl() call.

> But all I need for coresight is to have available the sink information
> and PMU configuration (in an upcoming patchset) by the time
> setup_aux() is called.  You are correct, this is a one time
> configuration and since the event isn't running there is no need for
> locking - I should be able to access the PMU when the ioctl is called.
>
> If you are fine with this bare-bone scenario and don't care much about
> usability in different situation, I'll do a respin with minimal
> functionality that cover my needs.

It doesn't have to be bare-bones, what I'm saying is that you shouldn't
need the event->drv_config, as you can directly call
pmu::config(new_config) (or config_sync(), but I'm guessing the _sync
part is redundant if you don't keep the configuration in 2 parts) from
the ioctl() and it should cover all your bases.

Regards,
--
Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> Hi Alex,

Hi Mathieu,

> On Tue, 3 Jul 2018 at 04:03, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>>
>> On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
>> > This patch follows what has been done for filters by adding an ioctl()
>> > option to communicate to the kernel arbitrary PMU specific configuration
>> > that don't fit in the conventional struct perf_event_attr to the kernel.
>>
>> Ok, so what *is* the PMU specific configuration that doesn't fit in the
>> attribute and needs to be re-configured by the driver using the generation
>> tracking?
>>
>
> In this patchset I'm am after the specification of sink information
> for each event, i.e what sink a CPU is supposed to use for the
> session.  I simply don't see putting something that PMU specific in
> the generic perf_event_attr structure.  I also intend to use the same
> ioctl mechanism to communicate complex tracer configuration for
> sequencers, counters and input events.  I don't see a nice way of
> doing that from the perf_event_attr, and that is even without thinking
> about the different flavours of tracers out there, all with their own
> features.

Yes, the sequencers and counters seem tricky. Here's a wild idea: can
the sequencer/counter configuration be expressed as an eBPF program? Or,
can an eBPF program be used to program those?

> I've looked around and the only clean way I found to support this is
> via an ioctl().  That way each tracer can easily identify the sink it
> should be using without smearing the perf_event_attr structure.  I
> would be happy to explore a different avenue should you think of
> something.

Yes, I also have something similar on my todo list and I was previously
thinking along the lines of pipe()/splice(). As in, you take the AUX
event file descriptor and feed it to the sink, at which point the trace
path is configured. I need to dig up the notes that I made back in the
day to continue this conversation in more concrete terms.

Regards,
--
Alex

Re: [PATCH 5/6] perf/core: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

On Wed, 4 Jul 2018 at 04:35, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
>
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
> > On Tue, 3 Jul 2018 at 04:57, Alexander Shishkin
> > <alexander.shishkin@linux.intel.com> wrote:
> >>
> >> On Tue, Jul 03, 2018 at 01:03:48PM +0300, Alexander Shishkin wrote:
> >> > On Mon, Jul 02, 2018 at 04:33:29PM -0600, Mathieu Poirier wrote:
> >> > > +/*
> >> > > + * PMU driver configuration works the same way as filter management above,
> >> > > + * but without the need to deal with memory mapping.  Driver configuration
> >> > > + * arrives through the SET_DRV_CONFIG ioctl() where it is validated and applied
> >> > > + * to the event.  When the PMU is ready it calls perf_event_drv_config_sync() to
> >> > > + * bring the configuration information within reach of the PMU.
> >> >
> >> > Wait a second. The reason why we dance around with the generations of filters
> >> > is the locking order of ctx::mutex vs mmap_sem. In an mmap path, where we're
> >> > notified about mapping changes, we're called under the latter, and we'd need
> >> > to grab the former to update the event configuration. In your case, the
> >> > update comes in via perf_ioctl(), where we're already holding the ctx::mutex,
> >> > so you can just kick the PMU right there, via an event_function_call() or
> >> > perf_event_stop(restart=1). In the latter case, your pmu::start() would just
> >> > grab the new configuration. Should also be about 90% less code. :)
> >>
> >> Also, since it affects the AUX buffer configuration, it is probably a one
> >> time ioctl command that you issue before you mmap the buffer. If that's
> >> the case, you don't even have to worry about stopping the event, as it
> >> shouldn't be running, because without the buffer perf_aux_output_begin()
> >> should fail and so should the pmu::add() iirc.
> >>
> >
> > The idea behind the current approach was to make the SET_DRV_CONFIG
> > ioctl() usable by other drivers where multiple ioctl() calls could be
> > performed while a session in ongoing.  I also opted to introduce a
> > _sync() function to let the PMU refresh its configuration at the time
> > of its own choosing rather than having to interrupt the session.
>
> Yes, but the times of PMU's own choosing would still be more or less
> limited to ->start()/->stop().
>
> You can also do an event_function_call(), which would call
> ->config_sync(), which would be free to decide what to do with the new
> information, up to and including doing a ->stop()/->start() sequence. My
> guess is that you'd want to do either of the following:
>  * decide to apply the new configuration immediately, and do the
>  start-stop thing,
>  * decide to defer the new configuration until the next ->start().
>
> Both should work via cross call directly from the ioctl() call.

Other than doing a cross call, do you see any advantage of using
event_function_call()?  For what I currently need the cross call is
not necessary.

>
> > But all I need for coresight is to have available the sink information
> > and PMU configuration (in an upcoming patchset) by the time
> > setup_aux() is called.  You are correct, this is a one time
> > configuration and since the event isn't running there is no need for
> > locking - I should be able to access the PMU when the ioctl is called.
> >
> > If you are fine with this bare-bone scenario and don't care much about
> > usability in different situation, I'll do a respin with minimal
> > functionality that cover my needs.
>
> It doesn't have to be bare-bones, what I'm saying is that you shouldn't
> need the event->drv_config, as you can directly call
> pmu::config(new_config) (or config_sync(), but I'm guessing the _sync
> part is redundant if you don't keep the configuration in 2 parts) from
> the ioctl() and it should cover all your bases.

I think I get your point - I will do another respin and we can go from there.

Thanks,
Mathieu

>
> Regards,
> --
> Alex