[PATCH 0/3] perf: Updates for address filters

[PATCH 0/3] perf: Updates for address filters

[Alexander Shishkin]

Hi Peter,

Here's a small update. One 'feature' that is added is kernel filters on
cpu events, so that one can trace scheduling paths and suchlike. While at
it, I also brushed up the filter structure a bit, iirc that's also what
Ingo wanted. And lastly there was one glitch in the filter parsing that
silently discarded filters when a kernel filter was SET_FILTER'ed on a
exclude_kernel==1 event.

Alexander Shishkin (3):
  perf, pt, coresight: Clean up address filter structure
  perf: Do error out on a kernel filter on an exclude_filter event
  perf: Allow kernel filters on cpu events

 arch/x86/events/intel/pt.c                       |  7 ++-
 drivers/hwtracing/coresight/coresight-etm-perf.c | 33 ++++----------
 include/linux/perf_event.h                       | 16 ++++---
 kernel/events/core.c                             | 55 +++++++++++++++---------
 4 files changed, 58 insertions(+), 53 deletions(-)

-- 
2.11.0

[PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Alexander Shishkin]

This is a cosmetic patch that deals with the address filter structure's
ambiguous fields 'filter' and 'range'. The former stands to mean that the
filter's *action* should be to filter the traces to its address range if
it's set or stop tracing if it's unset. This is confusing and hard on the
eyes, so this patch replaces it with 'action' enum. The 'range' field is
completely redundant (meaning that the filter is an address range as
opposed to a single address trigger), as we can use zero size to mean the
same thing.

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

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 5dba5468f8..5ac0911ac6 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1141,7 +1141,7 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
 
 	list_for_each_entry(filter, filters, entry) {
 		/* PT doesn't support single address triggers */
-		if (!filter->range || !filter->size)
+		if (!filter->size)
 			return -EOPNOTSUPP;
 
 		if (!filter->inode) {
@@ -1181,7 +1181,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
 
 		filters->filter[range].msr_a  = msr_a;
 		filters->filter[range].msr_b  = msr_b;
-		filters->filter[range].config = filter->filter ? 1 : 2;
+		if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
+			filters->filter[range].config = 1;
+		else
+			filters->filter[range].config = 2;
 		range++;
 	}
 
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 1774196902..62ae652664 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -392,35 +392,18 @@ static int etm_addr_filters_validate(struct list_head *filters)
 		if (++index > ETM_ADDR_CMP_MAX)
 			return -EOPNOTSUPP;
 
+		/* filter::size==0 means single address trigger */
+		if (filter->size)
+			range = true;
+		else
+			address = true;
+
 		/*
-		 * As taken from the struct perf_addr_filter documentation:
-		 *	@range:	1: range, 0: address
-		 *
 		 * At this time we don't allow range and start/stop filtering
 		 * to cohabitate, they have to be mutually exclusive.
 		 */
-		if ((filter->range == 1) && address)
+		if (range && address)
 			return -EOPNOTSUPP;
-
-		if ((filter->range == 0) && range)
-			return -EOPNOTSUPP;
-
-		/*
-		 * For range filtering, the second address in the address
-		 * range comparator needs to be higher than the first.
-		 * Invalid otherwise.
-		 */
-		if (filter->range && filter->size == 0)
-			return -EINVAL;
-
-		/*
-		 * Everything checks out with this filter, record what we've
-		 * received before moving on to the next one.
-		 */
-		if (filter->range)
-			range = true;
-		else
-			address = true;
 	}
 
 	return 0;
@@ -445,7 +428,7 @@ static void etm_addr_filters_sync(struct perf_event *event)
 			etm_filter->stop_addr = stop;
 			etm_filter->type = ETM_ADDR_TYPE_RANGE;
 		} else {
-			if (filter->filter == 1) {
+			if (filter->action == PERF_ADDR_FILTER_ACTION_START) {
 				etm_filter->start_addr = start;
 				etm_filter->type = ETM_ADDR_TYPE_START;
 			} else {
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 214c81588e..fcb37c81ca 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -457,14 +457,19 @@ struct pmu {
 	int (*filter_match)		(struct perf_event *event); /* optional */
 };
 
+enum perf_addr_filter_action_t {
+	PERF_ADDR_FILTER_ACTION_STOP = 0,
+	PERF_ADDR_FILTER_ACTION_START,
+	PERF_ADDR_FILTER_ACTION_FILTER = PERF_ADDR_FILTER_ACTION_START,
+};
+
 /**
  * struct perf_addr_filter - address range filter definition
  * @entry:	event's filter list linkage
  * @inode:	object file's inode for file-based filters
  * @offset:	filter range offset
- * @size:	filter range size
- * @range:	1: range, 0: address
- * @filter:	1: filter/start, 0: stop
+ * @size:	filter range size (size==0 means single address trigger)
+ * @action:	filter/start/stop
  *
  * This is a hardware-agnostic filter configuration as specified by the user.
  */
@@ -473,8 +478,7 @@ struct perf_addr_filter {
 	struct inode		*inode;
 	unsigned long		offset;
 	unsigned long		size;
-	unsigned int		range	: 1,
-				filter	: 1;
+	enum perf_addr_filter_action_t	action;
 };
 
 /**
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 458ff624d6..b422b5feee 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8141,7 +8141,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
  *  * for kernel addresses: <start address>[/<size>]
  *  * for object files:     <start address>[/<size>]@</path/to/object/file>
  *
- * if <size> is not specified, the range is treated as a single address.
+ * if <size> is not specified or is zero, the range is treated as a single
+ * address.
  */
 enum {
 	IF_ACT_NONE = -1,
@@ -8207,7 +8208,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 		switch (token) {
 		case IF_ACT_FILTER:
 		case IF_ACT_START:
-			filter->filter = 1;
+			filter->action = PERF_ADDR_FILTER_ACTION_FILTER;
 
 		case IF_ACT_STOP:
 			if (state != IF_STATE_ACTION)
@@ -8225,15 +8226,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			if (state != IF_STATE_SOURCE)
 				goto fail;
 
-			if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
-				filter->range = 1;
-
 			*args[0].to = 0;
 			ret = kstrtoul(args[0].from, 0, &filter->offset);
 			if (ret)
 				goto fail;
 
-			if (filter->range) {
+			if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
 				*args[1].to = 0;
 				ret = kstrtoul(args[1].from, 0, &filter->size);
 				if (ret)
@@ -8241,7 +8239,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			}
 
 			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
-				int fpos = filter->range ? 2 : 1;
+				int fpos = token == IF_SRC_FILE ? 2 : 1;
 
 				filename = match_strdup(&args[fpos]);
 				if (!filename) {
-- 
2.11.0

[PATCH 2/3] perf: Do error out on a kernel filter on an exclude_filter event

[Alexander Shishkin]

It is currently possible to configure a kernel address filter for a
event that excludes kernel from its traces (attr.exclude_kernel==1).

While in reality this doesn't make sense, the SET_FILTER ioctl() should
return a error in such case, currently it does not. Furthermore, it
will still silently discard the filter and any potentially valid filters
that came with it.

This patch makes the SET_FILTER ioctl() error out in such cases.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 kernel/events/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index b422b5feee..36770a13ef 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8261,6 +8261,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 		 * attribute.
 		 */
 		if (state == IF_STATE_END) {
+			ret = -EINVAL;
 			if (kernel && event->attr.exclude_kernel)
 				goto fail;
 
-- 
2.11.0

[PATCH 3/3] perf: Allow kernel filters on cpu events

[Alexander Shishkin]

While supporting file-based address filters for cpu events requires some
extra context switch handling, kernel address filters are easy, since the
kernel mapping is preserved across address spaces. It is also useful as
it permits tracing scheduling paths of the kernel.

This patch allows setting up kernel filters for cpu events.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/linux/perf_event.h |  2 ++
 kernel/events/core.c       | 42 ++++++++++++++++++++++++++++--------------
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fcb37c81ca..f4ea0600b2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -486,6 +486,7 @@ struct perf_addr_filter {
  * @list:	list of filters for this event
  * @lock:	spinlock that serializes accesses to the @list and event's
  *		(and its children's) filter generations.
+ * @nr_file_filters:	number of file-based filters
  *
  * A child event will use parent's @list (and therefore @lock), so they are
  * bundled together; see perf_event_addr_filters().
@@ -493,6 +494,7 @@ struct perf_addr_filter {
 struct perf_addr_filters_head {
 	struct list_head	list;
 	raw_spinlock_t		lock;
+	unsigned int		nr_file_filters;
 };
 
 /**
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 36770a13ef..2eeb8fec2f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8093,6 +8093,9 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
 	if (task == TASK_TOMBSTONE)
 		return;
 
+	if (!ifh->nr_file_filters)
+		return;
+
 	mm = get_task_mm(event->ctx->task);
 	if (!mm)
 		goto restart;
@@ -8269,6 +8272,18 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 				if (!filename)
 					goto fail;
 
+				/*
+				 * For now, we only support file-based filters
+				 * in per-task events; doing so for CPU-wide
+				 * events requires additional context switching
+				 * trickery, since same object code will be
+				 * mapped at different virtual addresses in
+				 * different processes.
+				 */
+				ret = -EOPNOTSUPP;
+				if (!event->ctx->task)
+					goto fail_free_name;
+
 				/* look up the path and grab its inode */
 				ret = kern_path(filename, LOOKUP_FOLLOW, &path);
 				if (ret)
@@ -8284,6 +8299,8 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 				    !S_ISREG(filter->inode->i_mode))
 					/* free_filters_list() will iput() */
 					goto fail;
+
+				event->addr_filters.nr_file_filters++;
 			}
 
 			/* ready to consume more filters */
@@ -8323,24 +8340,13 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str)
 	if (WARN_ON_ONCE(event->parent))
 		return -EINVAL;
 
-	/*
-	 * For now, we only support filtering in per-task events; doing so
-	 * for CPU-wide events requires additional context switching trickery,
-	 * since same object code will be mapped at different virtual
-	 * addresses in different processes.
-	 */
-	if (!event->ctx->task)
-		return -EOPNOTSUPP;
-
 	ret = perf_event_parse_addr_filter(event, filter_str, &filters);
 	if (ret)
-		return ret;
+		goto fail_clear_files;
 
 	ret = event->pmu->addr_filters_validate(&filters);
-	if (ret) {
-		free_filters_list(&filters);
-		return ret;
-	}
+	if (ret)
+		goto fail_free_filters;
 
 	/* remove existing filters, if any */
 	perf_addr_filters_splice(event, &filters);
@@ -8349,6 +8355,14 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str)
 	perf_event_for_each_child(event, perf_event_addr_filters_apply);
 
 	return ret;
+
+fail_free_filters:
+	free_filters_list(&filters);
+
+fail_clear_files:
+	event->addr_filters.nr_file_filters = 0;
+
+	return ret;
 }
 
 static int perf_event_set_filter(struct perf_event *event, void __user *arg)
-- 
2.11.0

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[kbuild test robot]

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

Hi Alexander,

[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.10-rc5 next-20170125]
[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/Alexander-Shishkin/perf-Updates-for-address-filters/20170126-175256
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/hwtracing/coresight/coresight-etm-perf.c: In function 'etm_addr_filters_sync':
>> drivers/hwtracing/coresight/coresight-etm-perf.c:426:13: error: 'struct perf_addr_filter' has no member named 'range'
      if (filter->range == 1) {
                ^~

vim +426 drivers/hwtracing/coresight/coresight-etm-perf.c

ca878b14 Mathieu Poirier 2016-08-25  420  
ca878b14 Mathieu Poirier 2016-08-25  421  	list_for_each_entry(filter, &head->list, entry) {
ca878b14 Mathieu Poirier 2016-08-25  422  		start = filter->offset + offs[i];
ca878b14 Mathieu Poirier 2016-08-25  423  		stop = start + filter->size;
ca878b14 Mathieu Poirier 2016-08-25  424  		etm_filter = &filters->etm_filter[i];
ca878b14 Mathieu Poirier 2016-08-25  425  
ca878b14 Mathieu Poirier 2016-08-25 @426  		if (filter->range == 1) {
ca878b14 Mathieu Poirier 2016-08-25  427  			etm_filter->start_addr = start;
ca878b14 Mathieu Poirier 2016-08-25  428  			etm_filter->stop_addr = stop;
ca878b14 Mathieu Poirier 2016-08-25  429  			etm_filter->type = ETM_ADDR_TYPE_RANGE;

:::::: The code at line 426 was first introduced by commit
:::::: ca878b14660c340287fb17d70950cfc09c6d698c coresight: etm-perf: configuring filters from perf core

:::::: TO: Mathieu Poirier <mathieu.poirier@linaro.org>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
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: 60054 bytes --]

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Alexander Shishkin]

kbuild test robot <lkp@intel.com> writes:

> Hi Alexander,
>
> [auto build test ERROR on tip/perf/core]
> [also build test ERROR on v4.10-rc5 next-20170125]
> [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/Alexander-Shishkin/perf-Updates-for-address-filters/20170126-175256
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
>
> All errors (new ones prefixed by >>):
>
>    drivers/hwtracing/coresight/coresight-etm-perf.c: In function 'etm_addr_filters_sync':
>>> drivers/hwtracing/coresight/coresight-etm-perf.c:426:13: error: 'struct perf_addr_filter' has no member named 'range'
>       if (filter->range == 1) {
>                 ^~

Right. Now it should also compile.

>From f50111d892a63721bef66eae39feadcd0194b163 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Mon, 23 Jan 2017 17:29:20 +0200
Subject: [PATCH v1] perf, pt, coresight: Clean up address filter structure

This is a cosmetic patch that deals with the address filter structure's
ambiguous fields 'filter' and 'range'. The former stands to mean that the
filter's *action* should be to filter the traces to its address range if
it's set or stop tracing if it's unset. This is confusing and hard on the
eyes, so this patch replaces it with 'action' enum. The 'range' field is
completely redundant (meaning that the filter is an address range as
opposed to a single address trigger), as we can use zero size to mean the
same thing.

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

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 5dba5468f8..5ac0911ac6 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1141,7 +1141,7 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
 
 	list_for_each_entry(filter, filters, entry) {
 		/* PT doesn't support single address triggers */
-		if (!filter->range || !filter->size)
+		if (!filter->size)
 			return -EOPNOTSUPP;
 
 		if (!filter->inode) {
@@ -1181,7 +1181,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
 
 		filters->filter[range].msr_a  = msr_a;
 		filters->filter[range].msr_b  = msr_b;
-		filters->filter[range].config = filter->filter ? 1 : 2;
+		if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
+			filters->filter[range].config = 1;
+		else
+			filters->filter[range].config = 2;
 		range++;
 	}
 
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 1774196902..f14bc5c73f 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -392,35 +392,18 @@ static int etm_addr_filters_validate(struct list_head *filters)
 		if (++index > ETM_ADDR_CMP_MAX)
 			return -EOPNOTSUPP;
 
+		/* filter::size==0 means single address trigger */
+		if (filter->size)
+			range = true;
+		else
+			address = true;
+
 		/*
-		 * As taken from the struct perf_addr_filter documentation:
-		 *	@range:	1: range, 0: address
-		 *
 		 * At this time we don't allow range and start/stop filtering
 		 * to cohabitate, they have to be mutually exclusive.
 		 */
-		if ((filter->range == 1) && address)
+		if (range && address)
 			return -EOPNOTSUPP;
-
-		if ((filter->range == 0) && range)
-			return -EOPNOTSUPP;
-
-		/*
-		 * For range filtering, the second address in the address
-		 * range comparator needs to be higher than the first.
-		 * Invalid otherwise.
-		 */
-		if (filter->range && filter->size == 0)
-			return -EINVAL;
-
-		/*
-		 * Everything checks out with this filter, record what we've
-		 * received before moving on to the next one.
-		 */
-		if (filter->range)
-			range = true;
-		else
-			address = true;
 	}
 
 	return 0;
@@ -440,12 +423,12 @@ static void etm_addr_filters_sync(struct perf_event *event)
 		stop = start + filter->size;
 		etm_filter = &filters->etm_filter[i];
 
-		if (filter->range == 1) {
+		if (filter->size) {
 			etm_filter->start_addr = start;
 			etm_filter->stop_addr = stop;
 			etm_filter->type = ETM_ADDR_TYPE_RANGE;
 		} else {
-			if (filter->filter == 1) {
+			if (filter->action == PERF_ADDR_FILTER_ACTION_START) {
 				etm_filter->start_addr = start;
 				etm_filter->type = ETM_ADDR_TYPE_START;
 			} else {
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 214c81588e..fcb37c81ca 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -457,14 +457,19 @@ struct pmu {
 	int (*filter_match)		(struct perf_event *event); /* optional */
 };
 
+enum perf_addr_filter_action_t {
+	PERF_ADDR_FILTER_ACTION_STOP = 0,
+	PERF_ADDR_FILTER_ACTION_START,
+	PERF_ADDR_FILTER_ACTION_FILTER = PERF_ADDR_FILTER_ACTION_START,
+};
+
 /**
  * struct perf_addr_filter - address range filter definition
  * @entry:	event's filter list linkage
  * @inode:	object file's inode for file-based filters
  * @offset:	filter range offset
- * @size:	filter range size
- * @range:	1: range, 0: address
- * @filter:	1: filter/start, 0: stop
+ * @size:	filter range size (size==0 means single address trigger)
+ * @action:	filter/start/stop
  *
  * This is a hardware-agnostic filter configuration as specified by the user.
  */
@@ -473,8 +478,7 @@ struct perf_addr_filter {
 	struct inode		*inode;
 	unsigned long		offset;
 	unsigned long		size;
-	unsigned int		range	: 1,
-				filter	: 1;
+	enum perf_addr_filter_action_t	action;
 };
 
 /**
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 458ff624d6..b422b5feee 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8141,7 +8141,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
  *  * for kernel addresses: <start address>[/<size>]
  *  * for object files:     <start address>[/<size>]@</path/to/object/file>
  *
- * if <size> is not specified, the range is treated as a single address.
+ * if <size> is not specified or is zero, the range is treated as a single
+ * address.
  */
 enum {
 	IF_ACT_NONE = -1,
@@ -8207,7 +8208,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 		switch (token) {
 		case IF_ACT_FILTER:
 		case IF_ACT_START:
-			filter->filter = 1;
+			filter->action = PERF_ADDR_FILTER_ACTION_FILTER;
 
 		case IF_ACT_STOP:
 			if (state != IF_STATE_ACTION)
@@ -8225,15 +8226,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			if (state != IF_STATE_SOURCE)
 				goto fail;
 
-			if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
-				filter->range = 1;
-
 			*args[0].to = 0;
 			ret = kstrtoul(args[0].from, 0, &filter->offset);
 			if (ret)
 				goto fail;
 
-			if (filter->range) {
+			if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
 				*args[1].to = 0;
 				ret = kstrtoul(args[1].from, 0, &filter->size);
 				if (ret)
@@ -8241,7 +8239,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			}
 
 			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
-				int fpos = filter->range ? 2 : 1;
+				int fpos = token == IF_SRC_FILE ? 2 : 1;
 
 				filename = match_strdup(&args[fpos]);
 				if (!filename) {
-- 
2.11.0

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

Hi Alex,


On Thu, Jan 26, 2017 at 11:40:55AM +0200, Alexander Shishkin wrote:
> This is a cosmetic patch that deals with the address filter structure's
> ambiguous fields 'filter' and 'range'. The former stands to mean that the
> filter's *action* should be to filter the traces to its address range if
> it's set or stop tracing if it's unset. This is confusing and hard on the
> eyes, so this patch replaces it with 'action' enum. The 'range' field is
> completely redundant (meaning that the filter is an address range as
> opposed to a single address trigger), as we can use zero size to mean the
> same thing.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  arch/x86/events/intel/pt.c                       |  7 +++--
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 33 ++++++------------------
>  include/linux/perf_event.h                       | 14 ++++++----
>  kernel/events/core.c                             | 12 ++++-----
>  4 files changed, 27 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 5dba5468f8..5ac0911ac6 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1141,7 +1141,7 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
>  
>  	list_for_each_entry(filter, filters, entry) {
>  		/* PT doesn't support single address triggers */
> -		if (!filter->range || !filter->size)
> +		if (!filter->size)
>  			return -EOPNOTSUPP;
>  
>  		if (!filter->inode) {
> @@ -1181,7 +1181,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
>  
>  		filters->filter[range].msr_a  = msr_a;
>  		filters->filter[range].msr_b  = msr_b;
> -		filters->filter[range].config = filter->filter ? 1 : 2;
> +		if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
> +			filters->filter[range].config = 1;
> +		else
> +			filters->filter[range].config = 2;
>  		range++;
>  	}
>  
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 1774196902..62ae652664 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -392,35 +392,18 @@ static int etm_addr_filters_validate(struct list_head *filters)
>  		if (++index > ETM_ADDR_CMP_MAX)
>  			return -EOPNOTSUPP;
>  
> +		/* filter::size==0 means single address trigger */
> +		if (filter->size)
> +			range = true;
> +		else
> +			address = true;
> +
>  		/*
> -		 * As taken from the struct perf_addr_filter documentation:
> -		 *	@range:	1: range, 0: address
> -		 *
>  		 * At this time we don't allow range and start/stop filtering
>  		 * to cohabitate, they have to be mutually exclusive.
>  		 */
> -		if ((filter->range == 1) && address)
> +		if (range && address)
>  			return -EOPNOTSUPP;
> -
> -		if ((filter->range == 0) && range)
> -			return -EOPNOTSUPP;
> -
> -		/*
> -		 * For range filtering, the second address in the address
> -		 * range comparator needs to be higher than the first.
> -		 * Invalid otherwise.
> -		 */
> -		if (filter->range && filter->size == 0)
> -			return -EINVAL;

This changes the behavior we used to have.  Now a range filter with a size of 0
will be treated as start filter rather than an error.  See below on a possible
way of fixing this.

> -
> -		/*
> -		 * Everything checks out with this filter, record what we've
> -		 * received before moving on to the next one.
> -		 */
> -		if (filter->range)
> -			range = true;
> -		else
> -			address = true;
>  	}
>  
>  	return 0;
> @@ -445,7 +428,7 @@ static void etm_addr_filters_sync(struct perf_event *event)
>  			etm_filter->stop_addr = stop;
>  			etm_filter->type = ETM_ADDR_TYPE_RANGE;
>  		} else {
> -			if (filter->filter == 1) {
> +			if (filter->action == PERF_ADDR_FILTER_ACTION_START) {
>  				etm_filter->start_addr = start;
>  				etm_filter->type = ETM_ADDR_TYPE_START;
>  			} else {
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 214c81588e..fcb37c81ca 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -457,14 +457,19 @@ struct pmu {
>  	int (*filter_match)		(struct perf_event *event); /* optional */
>  };
>  
> +enum perf_addr_filter_action_t {
> +	PERF_ADDR_FILTER_ACTION_STOP = 0,
> +	PERF_ADDR_FILTER_ACTION_START,
> +	PERF_ADDR_FILTER_ACTION_FILTER = PERF_ADDR_FILTER_ACTION_START,

If we are to embark on a renaming exercise I think
PERF_ADDR_FILTER_ACTION_FILTER should be renamed PERF_ADDR_FILTER_ACTION_RANGE.
In the end that's exactly what it is.

> +};
> +
>  /**
>   * struct perf_addr_filter - address range filter definition
>   * @entry:	event's filter list linkage
>   * @inode:	object file's inode for file-based filters
>   * @offset:	filter range offset
> - * @size:	filter range size
> - * @range:	1: range, 0: address
> - * @filter:	1: filter/start, 0: stop
> + * @size:	filter range size (size==0 means single address trigger)
> + * @action:	filter/start/stop
>   *
>   * This is a hardware-agnostic filter configuration as specified by the user.
>   */
> @@ -473,8 +478,7 @@ struct perf_addr_filter {
>  	struct inode		*inode;
>  	unsigned long		offset;
>  	unsigned long		size;
> -	unsigned int		range	: 1,
> -				filter	: 1;
> +	enum perf_addr_filter_action_t	action;
>  };
>  
>  /**
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 458ff624d6..b422b5feee 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8141,7 +8141,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>   *  * for kernel addresses: <start address>[/<size>]
>   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
>   *
> - * if <size> is not specified, the range is treated as a single address.
> + * if <size> is not specified or is zero, the range is treated as a single
> + * address.
>   */
>  enum {
>  	IF_ACT_NONE = -1,
> @@ -8207,7 +8208,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>  		switch (token) {
>  		case IF_ACT_FILTER:
>  		case IF_ACT_START:
> -			filter->filter = 1;
> +			filter->action = PERF_ADDR_FILTER_ACTION_FILTER;

To fix the problem mentioned above:

1) Reorder the "if_tokens" match table to list stop, start and filter, in that
order.

2) Then for this case statement filter->action = token;

>  
>  		case IF_ACT_STOP:
>  			if (state != IF_STATE_ACTION)
> @@ -8225,15 +8226,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>  			if (state != IF_STATE_SOURCE)
>  				goto fail;
>  
> -			if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
> -				filter->range = 1;
> -
>  			*args[0].to = 0;
>  			ret = kstrtoul(args[0].from, 0, &filter->offset);
>  			if (ret)
>  				goto fail;
>  
> -			if (filter->range) {
> +			if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {

                        if (filter->action == PERF_ADDR_FILTER_ACTION_RANGE)

>  				*args[1].to = 0;
>  				ret = kstrtoul(args[1].from, 0, &filter->size);
>  				if (ret)
> @@ -8241,7 +8239,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>  			}
>  
>  			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
> -				int fpos = filter->range ? 2 : 1;
> +				int fpos = token == IF_SRC_FILE ? 2 : 1;
>  
>  				filename = match_strdup(&args[fpos]);
>  				if (!filename) {
> -- 
> 2.11.0
> 

Re: [PATCH 2/3] perf: Do error out on a kernel filter on an exclude_filter event

[Mathieu Poirier]

On Thu, Jan 26, 2017 at 11:40:56AM +0200, Alexander Shishkin wrote:
> It is currently possible to configure a kernel address filter for a
> event that excludes kernel from its traces (attr.exclude_kernel==1).
> 
> While in reality this doesn't make sense, the SET_FILTER ioctl() should
> return a error in such case, currently it does not. Furthermore, it
> will still silently discard the filter and any potentially valid filters
> that came with it.
> 
> This patch makes the SET_FILTER ioctl() error out in such cases.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  kernel/events/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index b422b5feee..36770a13ef 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8261,6 +8261,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>  		 * attribute.
>  		 */
>  		if (state == IF_STATE_END) {
> +			ret = -EINVAL;
>  			if (kernel && event->attr.exclude_kernel)
>  				goto fail;

Yes, I stumbled on that when implementing filters on CS and could swear I had a
patch for it.  It obviously never got sent out.

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>  
> -- 
> 2.11.0
> 

Re: [PATCH 3/3] perf: Allow kernel filters on cpu events

[Mathieu Poirier]

On Thu, Jan 26, 2017 at 11:40:57AM +0200, Alexander Shishkin wrote:
> While supporting file-based address filters for cpu events requires some
> extra context switch handling, kernel address filters are easy, since the
> kernel mapping is preserved across address spaces. It is also useful as
> it permits tracing scheduling paths of the kernel.
> 
> This patch allows setting up kernel filters for cpu events.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  include/linux/perf_event.h |  2 ++
>  kernel/events/core.c       | 42 ++++++++++++++++++++++++++++--------------
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index fcb37c81ca..f4ea0600b2 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -486,6 +486,7 @@ struct perf_addr_filter {
>   * @list:	list of filters for this event
>   * @lock:	spinlock that serializes accesses to the @list and event's
>   *		(and its children's) filter generations.
> + * @nr_file_filters:	number of file-based filters
>   *
>   * A child event will use parent's @list (and therefore @lock), so they are
>   * bundled together; see perf_event_addr_filters().
> @@ -493,6 +494,7 @@ struct perf_addr_filter {
>  struct perf_addr_filters_head {
>  	struct list_head	list;
>  	raw_spinlock_t		lock;
> +	unsigned int		nr_file_filters;
>  };
>  
>  /**
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 36770a13ef..2eeb8fec2f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8093,6 +8093,9 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>  	if (task == TASK_TOMBSTONE)
>  		return;
>  
> +	if (!ifh->nr_file_filters)
> +		return;

Is this mandatory or an optimisation to avoid circling through a list of filters
that don't included user space files?

> +
>  	mm = get_task_mm(event->ctx->task);
>  	if (!mm)
>  		goto restart;
> @@ -8269,6 +8272,18 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>  				if (!filename)
>  					goto fail;
>  
> +				/*
> +				 * For now, we only support file-based filters
> +				 * in per-task events; doing so for CPU-wide
> +				 * events requires additional context switching
> +				 * trickery, since same object code will be
> +				 * mapped at different virtual addresses in
> +				 * different processes.
> +				 */
> +				ret = -EOPNOTSUPP;
> +				if (!event->ctx->task)
> +					goto fail_free_name;
> +
>  				/* look up the path and grab its inode */
>  				ret = kern_path(filename, LOOKUP_FOLLOW, &path);
>  				if (ret)
> @@ -8284,6 +8299,8 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>  				    !S_ISREG(filter->inode->i_mode))
>  					/* free_filters_list() will iput() */
>  					goto fail;
> +
> +				event->addr_filters.nr_file_filters++;
>  			}
>  
>  			/* ready to consume more filters */
> @@ -8323,24 +8340,13 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str)
>  	if (WARN_ON_ONCE(event->parent))
>  		return -EINVAL;
>  
> -	/*
> -	 * For now, we only support filtering in per-task events; doing so
> -	 * for CPU-wide events requires additional context switching trickery,
> -	 * since same object code will be mapped at different virtual
> -	 * addresses in different processes.
> -	 */
> -	if (!event->ctx->task)
> -		return -EOPNOTSUPP;
> -
>  	ret = perf_event_parse_addr_filter(event, filter_str, &filters);
>  	if (ret)
> -		return ret;
> +		goto fail_clear_files;
>  
>  	ret = event->pmu->addr_filters_validate(&filters);
> -	if (ret) {
> -		free_filters_list(&filters);
> -		return ret;
> -	}
> +	if (ret)
> +		goto fail_free_filters;
>  
>  	/* remove existing filters, if any */
>  	perf_addr_filters_splice(event, &filters);
> @@ -8349,6 +8355,14 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str)
>  	perf_event_for_each_child(event, perf_event_addr_filters_apply);
>  
>  	return ret;
> +
> +fail_free_filters:
> +	free_filters_list(&filters);
> +
> +fail_clear_files:
> +	event->addr_filters.nr_file_filters = 0;
> +
> +	return ret;
>  }
>  
>  static int perf_event_set_filter(struct perf_event *event, void __user *arg)
> -- 
> 2.11.0
> 

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Alexander Shishkin]

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

> Hi Alex,

Hi Mathieu,

> This changes the behavior we used to have.  Now a range filter with a size of 0
> will be treated as start filter rather than an error.  See below on a possible
> way of fixing this.

Not really. Currently we have 2 drivers using this and both reject the
type=range&&size==0 filters with either -EOPNOTSUPP or -EINVAL. With
this change, PT will still reject it as it doesn't support single
address triggers, but Coresight will treat it as if it was a single
address filter. Which makes sense, because that's what a range of size
zero is. Note, that a range that covers one instruction has to be at
least size==1 (and I'm guessing size==4 for Coresight, but I may be
wrong).

So yes, this does change the existing behavior, but in doing so it
removes the ambiguity of zero sized ranges.

>                         if (filter->action == PERF_ADDR_FILTER_ACTION_RANGE)

But "range" is not an action, it's a type of a filter. It determines the
condition that triggers an action. An action, however, is what we do
when the condition comes true.

Regards,
--
Alex

Re: [PATCH 3/3] perf: Allow kernel filters on cpu events

[Alexander Shishkin]

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

> On Thu, Jan 26, 2017 at 11:40:57AM +0200, Alexander Shishkin wrote:
>> +	if (!ifh->nr_file_filters)
>> +		return;
>
> Is this mandatory or an optimisation to avoid circling through a list of filters
> that don't included user space files?

It's both. It stems from the fact that the remainder of this function
relies on ctx::task not being NULL, which is not the case with cpu
contexts and now that we've enabled address filters for such contexts,
it's a problem. So checking for !task would have done the trick here,
but this way we'll also avoid going down this path for task contexts
in the absence of file-based filters. In particular, grabbing the mmap
semaphore and filters spinlock we can do without.

Regards,
--
Alex

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

On 27 January 2017 at 05:12, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> Hi Alex,
>
> Hi Mathieu,
>
>> This changes the behavior we used to have.  Now a range filter with a size of 0
>> will be treated as start filter rather than an error.  See below on a possible
>> way of fixing this.
>
> Not really. Currently we have 2 drivers using this and both reject the
> type=range&&size==0 filters with either -EOPNOTSUPP or -EINVAL. With
> this change, PT will still reject it as it doesn't support single
> address triggers, but Coresight will treat it as if it was a single
> address filter.

Hence my statement about a change in behaviour.

> Which makes sense, because that's what a range of size
> zero is. Note, that a range that covers one instruction has to be at
> least size==1 (and I'm guessing size==4 for Coresight, but I may be
> wrong).
>
> So yes, this does change the existing behavior, but in doing so it
> removes the ambiguity of zero sized ranges.

Specifying a size of zero with a range filter is wrong and as such
should be treated as an error, which is what the current code is
doing.  If people want a start filter they can use the syntax required
for that.  In my opinion treating a range filter with a size zero as a
start filter is adding intelligence to the machine, something that
should probably be avoided.

>
>>                         if (filter->action == PERF_ADDR_FILTER_ACTION_RANGE)
>
> But "range" is not an action, it's a type of a filter. It determines the
> condition that triggers an action. An action, however, is what we do
> when the condition comes true.

Then filter->action could be renamed 'type'.  In the end filters on PT
are range filters, the same way they are on CS.  But changing the
naming convention is a matter of personal opinion - I am fine with
what we currently have.

On the flip side reordering the fields in the 'if_tokens' match table
would allow to set the filters properly in
perf_event_parse_addr_filter(), keep the current behaviour intact and
get rid of filter->range.

Thanks,
Mathieu

>
> Regards,
> --
> Alex

Re: [PATCH 3/3] perf: Allow kernel filters on cpu events

[Mathieu Poirier]

On 27 January 2017 at 05:31, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On Thu, Jan 26, 2017 at 11:40:57AM +0200, Alexander Shishkin wrote:
>>> +    if (!ifh->nr_file_filters)
>>> +            return;
>>
>> Is this mandatory or an optimisation to avoid circling through a list of filters
>> that don't included user space files?
>
> It's both. It stems from the fact that the remainder of this function
> relies on ctx::task not being NULL, which is not the case with cpu
> contexts and now that we've enabled address filters for such contexts,
> it's a problem. So checking for !task would have done the trick here,
> but this way we'll also avoid going down this path for task contexts
> in the absence of file-based filters. In particular, grabbing the mmap
> semaphore and filters spinlock we can do without.

Yes, I see it now.

Do you have bigger plans for ->nr_file_filters?  If the purpose is
only to indicate the presence of user space files then it should be a
'bool' type (and probably changed to ->file_filters).

Thanks,
Mathieu

>
> Regards,
> --
> Alex

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Alexander Shishkin]

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

> On 27 January 2017 at 05:12, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> But "range" is not an action, it's a type of a filter. It determines the
>> condition that triggers an action. An action, however, is what we do
>> when the condition comes true.
>
> Then filter->action could be renamed 'type'.

No. Again, *action* is what we *do*. *Type* is *how* we detect that
something needs to be done.

> In the end filters on PT
> are range filters, the same way they are on CS.  But changing the

No. The CS driver supports both single address and address range
filters at least acconding to my reading of the code. Now that I look
more at it, I see that it also gets the range filters wrong: it
disregards filter->filter for range filters, assuming that since it's a
range, it means that the user wants to trace what's in the range
(filter->filter == 1), but it may also mean "stop if you end up in this
range" (filter->filter == 0). The fact that the CS driver gets it wrong
just proves the point that "filter->filter" is confusing and misleading
and needs to be replaced.

In the case of CS, I think that a -EOPNOTSUPP is also appropriate for
the type==range&&action==stop combination.

Regards,
--
Alex

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

)

On 1 February 2017 at 05:46, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 27 January 2017 at 05:12, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> But "range" is not an action, it's a type of a filter. It determines the
>>> condition that triggers an action. An action, however, is what we do
>>> when the condition comes true.
>>
>> Then filter->action could be renamed 'type'.
>
> No. Again, *action* is what we *do*. *Type* is *how* we detect that
> something needs to be done.

If this is what you want to convey then

+ * @action:    filter/start/stop

needs to be fixed.  This can be interpreted as "use range filter,
start filter or stop filter" - which is exactly what I did.  Something
like

+ * @action:    1: start filtering 0: stop filtering

will avoid any confusion.

>
>> In the end filters on PT
>> are range filters, the same way they are on CS.  But changing the
>
> No. The CS driver supports both single address and address range
> filters at least acconding to my reading of the code. Now that I look
> more at it, I see that it also gets the range filters wrong: it
> disregards filter->filter for range filters, assuming that since it's a
> range, it means that the user wants to trace what's in the range
> (filter->filter == 1), but it may also mean "stop if you end up in this
> range" (filter->filter == 0).

Exactly.  The code does the right thing based on my interpretation of
the comment found in the code:

* @range:      1: range, 0: address
* @filter:     1: filter/start, 0: stop

That is @range to determine if we are using a range or an address
filter and @filter to specify what kind of address filter to use
(start or stop).  Ignoring range filters when ->filter == 0 was done
on purpose as I simply couldn't see how to fit it in.

> The fact that the CS driver gets it wrong
> just proves the point that "filter->filter" is confusing and misleading
> and needs to be replaced.
>

I could not agree more.

On the flip side it doesn't change anything to my original argument:
the code should not be made to be smart.  If a range filter is used
then a size of zero should be treated as an error.

To move forward please keep the current functionality on the CS side,
i.e return -EINVAL when a size of zero is used with a range filter.
Once it is queued I'll send a set of patches to support the exclusion
of address ranges.

> In the case of CS, I think that a -EOPNOTSUPP is also appropriate for
> the type==range&&action==stop combination.

That will also be part of said patches.

Thanks,
Mathieu

>
> Regards,
> --
> Alex

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

On 1 February 2017 at 14:33, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> )
>
> On 1 February 2017 at 05:46, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>>
>>> On 27 January 2017 at 05:12, Alexander Shishkin
>>> <alexander.shishkin@linux.intel.com> wrote:
>>>> But "range" is not an action, it's a type of a filter. It determines the
>>>> condition that triggers an action. An action, however, is what we do
>>>> when the condition comes true.
>>>
>>> Then filter->action could be renamed 'type'.
>>
>> No. Again, *action* is what we *do*. *Type* is *how* we detect that
>> something needs to be done.
>
> If this is what you want to convey then
>
> + * @action:    filter/start/stop
>
> needs to be fixed.  This can be interpreted as "use range filter,
> start filter or stop filter" - which is exactly what I did.  Something
> like
>
> + * @action:    1: start filtering 0: stop filtering
>
> will avoid any confusion.
>
>>
>>> In the end filters on PT
>>> are range filters, the same way they are on CS.  But changing the
>>
>> No. The CS driver supports both single address and address range
>> filters at least acconding to my reading of the code. Now that I look
>> more at it, I see that it also gets the range filters wrong: it
>> disregards filter->filter for range filters, assuming that since it's a
>> range, it means that the user wants to trace what's in the range
>> (filter->filter == 1), but it may also mean "stop if you end up in this
>> range" (filter->filter == 0).
>
> Exactly.  The code does the right thing based on my interpretation of
> the comment found in the code:
>
> * @range:      1: range, 0: address
> * @filter:     1: filter/start, 0: stop
>
> That is @range to determine if we are using a range or an address
> filter and @filter to specify what kind of address filter to use
> (start or stop).  Ignoring range filters when ->filter == 0 was done
> on purpose as I simply couldn't see how to fit it in.
>
>> The fact that the CS driver gets it wrong
>> just proves the point that "filter->filter" is confusing and misleading
>> and needs to be replaced.
>>
>
> I could not agree more.
>
> On the flip side it doesn't change anything to my original argument:
> the code should not be made to be smart.  If a range filter is used
> then a size of zero should be treated as an error.
>
> To move forward please keep the current functionality on the CS side,
> i.e return -EINVAL when a size of zero is used with a range filter.
> Once it is queued I'll send a set of patches to support the exclusion
> of address ranges.
>
>> In the case of CS, I think that a -EOPNOTSUPP is also appropriate for
>> the type==range&&action==stop combination.
>
> That will also be part of said patches.
>
> Thanks,
> Mathieu

Furthermore...

static const match_table_t if_tokens = {
         { IF_ACT_FILTER,        "filter" },
         { IF_ACT_START,         "start" },
         { IF_ACT_STOP,          "stop" },
         { IF_SRC_FILE,          "%u/%u@%s" },
         { IF_SRC_KERNEL,        "%u/%u" },
         { IF_SRC_FILEADDR,      "%u@%s" },
         { IF_SRC_KERNELADDR,    "%u" },
         { IF_ACT_NONE,          NULL },
};

Do we have two different syntax to specify the same behaviour?

For example we have:

--filter 'start 0x80082570/0x644'

and

--filter 'filter 0x80082570/0x644'

Both will end up with filter->filter == 1 and filter->range == 1.

The same will be true for:

--filter 'start 0x80082570'

and

--filter 'filter 0x80082570'

ends up with filter->filter == 1 and filter->range == 0.

>
>>
>> Regards,
>> --
>> Alex

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Alexander Shishkin]

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

> Do we have two different syntax to specify the same behaviour?
>
> For example we have:
>
> --filter 'start 0x80082570/0x644'
>
> and
>
> --filter 'filter 0x80082570/0x644'
>
> Both will end up with filter->filter == 1 and filter->range == 1.

This is another reason why enum action is needed. The difference between
'start' and 'filter' is that the former means "start tracing when you
enter this region until something else stops it"; the latter means
"trace only inside this region" (that is, start tracing when you branch
inside this region and stop when you branch outside). They cannot be
treated interchangeably as I originally though. PT supports 'filter', CS
supports 'start', if I remember right. So we should make sure to
-EOPNOTSUPP things that we don't actually support.

Regards,
--
Alex

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Alexander Shishkin]

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

> If this is what you want to convey then
>
> + * @action:    filter/start/stop
>
> needs to be fixed.  This can be interpreted as "use range filter,
> start filter or stop filter" - which is exactly what I did.  Something
> like

I was beginning to think that the 'correct' way only existed in my head,
but then I noticed elsewhere there's a comment:

| where ACTION is one of the
|  * "filter": limit the trace to this region
|  * "start": start tracing from this address
|  * "stop": stop tracing at this address/region;

so it is sort of explained. Maybe I also need to spell it out in the
structure. And the man page, of course, when I get to it.

>> The fact that the CS driver gets it wrong
>> just proves the point that "filter->filter" is confusing and misleading
>> and needs to be replaced.
>>
>
> I could not agree more.
>
> On the flip side it doesn't change anything to my original argument:
> the code should not be made to be smart.  If a range filter is used
> then a size of zero should be treated as an error.

It actually does, because when you say 'range filter' you really mean
'filter filter' and we need to get this difference straight as I also
mentioned in the other email. ACTION=="filter" does not make sense with
size==0, this much is true. The other two are fine, though.

So considering all of the above, I have amended the patch to something
like the following. I'm not 100% sure that the CS side is accurate, but
I'm hoping you can have a look.

Side note: while looking through address comparator code, I noticed your
catch-all filter gets the end address wrong if compiled in 64bit mode.

>From 0a549c016591b470fc46b79664ec761db1db6666 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Mon, 23 Jan 2017 17:29:20 +0200
Subject: [PATCH] perf, pt, coresight: Clean up address filter structure

$WRITEME

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 arch/x86/events/intel/pt.c                       | 13 ++++--
 drivers/hwtracing/coresight/coresight-etm-perf.c | 59 +++++++++++-------------
 include/linux/perf_event.h                       | 14 ++++--
 kernel/events/core.c                             | 26 +++++++----
 4 files changed, 63 insertions(+), 49 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 1c1b9fe705..f693c8ae75 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1100,8 +1100,12 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
 	int range = 0;
 
 	list_for_each_entry(filter, filters, entry) {
-		/* PT doesn't support single address triggers */
-		if (!filter->range || !filter->size)
+		/*
+		 * PT doesn't support single address triggers and
+		 * 'start' filters.
+		 */
+		if (!filter->size ||
+		    filter->action == PERF_ADDR_FILTER_ACTION_START)
 			return -EOPNOTSUPP;
 
 		if (!filter->inode) {
@@ -1141,7 +1145,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
 
 		filters->filter[range].msr_a  = msr_a;
 		filters->filter[range].msr_b  = msr_b;
-		filters->filter[range].config = filter->filter ? 1 : 2;
+		if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
+			filters->filter[range].config = 1;
+		else
+			filters->filter[range].config = 2;
 		range++;
 	}
 
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 1774196902..70eaa74dc2 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -392,35 +392,26 @@ static int etm_addr_filters_validate(struct list_head *filters)
 		if (++index > ETM_ADDR_CMP_MAX)
 			return -EOPNOTSUPP;
 
+		/* filter::size==0 means single address trigger */
+		if (filter->size) {
+			/*
+			 * The existing code relies on START/STOP filters
+			 * being address filters.
+			 */
+			if (filter->action == PERF_ADDR_FILTER_ACTION_START ||
+			    filter->action == PERF_ADDR_FILTER_ACTION_STOP)
+				return -EOPNOTSUPP;
+
+			range = true;
+		} else
+			address = true;
+
 		/*
-		 * As taken from the struct perf_addr_filter documentation:
-		 *	@range:	1: range, 0: address
-		 *
 		 * At this time we don't allow range and start/stop filtering
 		 * to cohabitate, they have to be mutually exclusive.
 		 */
-		if ((filter->range == 1) && address)
+		if (range && address)
 			return -EOPNOTSUPP;
-
-		if ((filter->range == 0) && range)
-			return -EOPNOTSUPP;
-
-		/*
-		 * For range filtering, the second address in the address
-		 * range comparator needs to be higher than the first.
-		 * Invalid otherwise.
-		 */
-		if (filter->range && filter->size == 0)
-			return -EINVAL;
-
-		/*
-		 * Everything checks out with this filter, record what we've
-		 * received before moving on to the next one.
-		 */
-		if (filter->range)
-			range = true;
-		else
-			address = true;
 	}
 
 	return 0;
@@ -440,18 +431,20 @@ static void etm_addr_filters_sync(struct perf_event *event)
 		stop = start + filter->size;
 		etm_filter = &filters->etm_filter[i];
 
-		if (filter->range == 1) {
+		switch (filter->action) {
+		case PERF_ADDR_FILTER_ACTION_FILTER:
 			etm_filter->start_addr = start;
 			etm_filter->stop_addr = stop;
 			etm_filter->type = ETM_ADDR_TYPE_RANGE;
-		} else {
-			if (filter->filter == 1) {
-				etm_filter->start_addr = start;
-				etm_filter->type = ETM_ADDR_TYPE_START;
-			} else {
-				etm_filter->stop_addr = stop;
-				etm_filter->type = ETM_ADDR_TYPE_STOP;
-			}
+			break;
+		case PERF_ADDR_FILTER_ACTION_START:
+			etm_filter->start_addr = start;
+			etm_filter->type = ETM_ADDR_TYPE_START;
+			break;
+		case PERF_ADDR_FILTER_ACTION_STOP:
+			etm_filter->stop_addr = stop;
+			etm_filter->type = ETM_ADDR_TYPE_STOP;
+			break;
 		}
 		i++;
 	}
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1432df5a82..c09aff84f7 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -457,14 +457,19 @@ struct pmu {
 	int (*filter_match)		(struct perf_event *event); /* optional */
 };
 
+enum perf_addr_filter_action_t {
+	PERF_ADDR_FILTER_ACTION_STOP = 0,
+	PERF_ADDR_FILTER_ACTION_START,
+	PERF_ADDR_FILTER_ACTION_FILTER,
+};
+
 /**
  * struct perf_addr_filter - address range filter definition
  * @entry:	event's filter list linkage
  * @inode:	object file's inode for file-based filters
  * @offset:	filter range offset
- * @size:	filter range size
- * @range:	1: range, 0: address
- * @filter:	1: filter/start, 0: stop
+ * @size:	filter range size (size==0 means single address trigger)
+ * @action:	filter/start/stop
  *
  * This is a hardware-agnostic filter configuration as specified by the user.
  */
@@ -473,8 +478,7 @@ struct perf_addr_filter {
 	struct inode		*inode;
 	unsigned long		offset;
 	unsigned long		size;
-	unsigned int		range	: 1,
-				filter	: 1;
+	enum perf_addr_filter_action_t	action;
 };
 
 /**
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 39106ae61b..d7a11faac1 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8194,7 +8194,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
  *  * for kernel addresses: <start address>[/<size>]
  *  * for object files:     <start address>[/<size>]@</path/to/object/file>
  *
- * if <size> is not specified, the range is treated as a single address.
+ * if <size> is not specified or is zero, the range is treated as a single
+ * address; not valid for ACTION=="filter".
  */
 enum {
 	IF_ACT_NONE = -1,
@@ -8244,6 +8245,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 		return -ENOMEM;
 
 	while ((start = strsep(&fstr, " ,\n")) != NULL) {
+		static const enum perf_addr_filter_action_t actions[] = {
+			[IF_ACT_FILTER]	= PERF_ADDR_FILTER_ACTION_FILTER,
+			[IF_ACT_START]	= PERF_ADDR_FILTER_ACTION_START,
+			[IF_ACT_STOP]	= PERF_ADDR_FILTER_ACTION_STOP,
+		};
 		ret = -EINVAL;
 
 		if (!*start)
@@ -8260,12 +8266,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 		switch (token) {
 		case IF_ACT_FILTER:
 		case IF_ACT_START:
-			filter->filter = 1;
-
 		case IF_ACT_STOP:
 			if (state != IF_STATE_ACTION)
 				goto fail;
 
+			filter->action = actions[token];
 			state = IF_STATE_SOURCE;
 			break;
 
@@ -8278,15 +8283,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			if (state != IF_STATE_SOURCE)
 				goto fail;
 
-			if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
-				filter->range = 1;
-
 			*args[0].to = 0;
 			ret = kstrtoul(args[0].from, 0, &filter->offset);
 			if (ret)
 				goto fail;
 
-			if (filter->range) {
+			if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
 				*args[1].to = 0;
 				ret = kstrtoul(args[1].from, 0, &filter->size);
 				if (ret)
@@ -8294,7 +8296,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			}
 
 			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
-				int fpos = filter->range ? 2 : 1;
+				int fpos = token == IF_SRC_FILE ? 2 : 1;
 
 				filename = match_strdup(&args[fpos]);
 				if (!filename) {
@@ -8319,6 +8321,14 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			if (kernel && event->attr.exclude_kernel)
 				goto fail;
 
+			/*
+			 * ACTION "filter" must have a non-zero length region
+			 * specified.
+			 */
+			if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER &&
+			    !filter->size)
+				goto fail;
+
 			if (!kernel) {
 				if (!filename)
 					goto fail;
-- 
2.11.0

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

On 2 February 2017 at 03:42, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> Do we have two different syntax to specify the same behaviour?
>>
>> For example we have:
>>
>> --filter 'start 0x80082570/0x644'
>>
>> and
>>
>> --filter 'filter 0x80082570/0x644'
>>
>> Both will end up with filter->filter == 1 and filter->range == 1.
>
> This is another reason why enum action is needed. The difference between
> 'start' and 'filter' is that the former means "start tracing when you
> enter this region until something else stops it";

And what is the "something else here"?

> the latter means
> "trace only inside this region" (that is, start tracing when you branch
> inside this region and stop when you branch outside).

That is indeed how range filters work on CS.

> They cannot be
> treated interchangeably as I originally though. PT supports 'filter', CS
> supports 'start', if I remember right. So we should make sure to
> -EOPNOTSUPP things that we don't actually support.

I already published slides at 2 conferences that uses "filter" for
range filters.  On CS I will have to continue using "filter" and
"start" (when specified with a size element) as one and the same.

>
> Regards,
> --
> Alex

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

On 2 February 2017 at 09:22, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> If this is what you want to convey then
>>
>> + * @action:    filter/start/stop
>>
>> needs to be fixed.  This can be interpreted as "use range filter,
>> start filter or stop filter" - which is exactly what I did.  Something
>> like
>
> I was beginning to think that the 'correct' way only existed in my head,
> but then I noticed elsewhere there's a comment:
>
> | where ACTION is one of the
> |  * "filter": limit the trace to this region
> |  * "start": start tracing from this address
> |  * "stop": stop tracing at this address/region;
>
> so it is sort of explained. Maybe I also need to spell it out in the
> structure. And the man page, of course, when I get to it.
>
>>> The fact that the CS driver gets it wrong
>>> just proves the point that "filter->filter" is confusing and misleading
>>> and needs to be replaced.
>>>
>>
>> I could not agree more.
>>
>> On the flip side it doesn't change anything to my original argument:
>> the code should not be made to be smart.  If a range filter is used
>> then a size of zero should be treated as an error.
>
> It actually does, because when you say 'range filter' you really mean
> 'filter filter' and we need to get this difference straight as I also
> mentioned in the other email. ACTION=="filter" does not make sense with
> size==0, this much is true.

I've always been in agreement with that statement.

> The other two are fine, though.
>
> So considering all of the above, I have amended the patch to something
> like the following. I'm not 100% sure that the CS side is accurate, but
> I'm hoping you can have a look.
>
> Side note: while looking through address comparator code, I noticed your
> catch-all filter gets the end address wrong if compiled in 64bit mode.

I'm interested in your point of view on this - can you elaborate?

>
> From 0a549c016591b470fc46b79664ec761db1db6666 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Mon, 23 Jan 2017 17:29:20 +0200
> Subject: [PATCH] perf, pt, coresight: Clean up address filter structure
>
> $WRITEME
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  arch/x86/events/intel/pt.c                       | 13 ++++--
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 59 +++++++++++-------------
>  include/linux/perf_event.h                       | 14 ++++--
>  kernel/events/core.c                             | 26 +++++++----
>  4 files changed, 63 insertions(+), 49 deletions(-)
>
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 1c1b9fe705..f693c8ae75 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1100,8 +1100,12 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
>         int range = 0;
>
>         list_for_each_entry(filter, filters, entry) {
> -               /* PT doesn't support single address triggers */
> -               if (!filter->range || !filter->size)
> +               /*
> +                * PT doesn't support single address triggers and
> +                * 'start' filters.
> +                */
> +               if (!filter->size ||
> +                   filter->action == PERF_ADDR_FILTER_ACTION_START)
>                         return -EOPNOTSUPP;
>
>                 if (!filter->inode) {
> @@ -1141,7 +1145,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
>
>                 filters->filter[range].msr_a  = msr_a;
>                 filters->filter[range].msr_b  = msr_b;
> -               filters->filter[range].config = filter->filter ? 1 : 2;
> +               if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
> +                       filters->filter[range].config = 1;
> +               else
> +                       filters->filter[range].config = 2;
>                 range++;
>         }
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 1774196902..70eaa74dc2 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -392,35 +392,26 @@ static int etm_addr_filters_validate(struct list_head *filters)
>                 if (++index > ETM_ADDR_CMP_MAX)
>                         return -EOPNOTSUPP;
>
> +               /* filter::size==0 means single address trigger */
> +               if (filter->size) {
> +                       /*
> +                        * The existing code relies on START/STOP filters
> +                        * being address filters.
> +                        */

The above comment is not needed.

> +                       if (filter->action == PERF_ADDR_FILTER_ACTION_START ||
> +                           filter->action == PERF_ADDR_FILTER_ACTION_STOP)
> +                               return -EOPNOTSUPP;
> +
> +                       range = true;
> +               } else
> +                       address = true;
> +
>                 /*
> -                * As taken from the struct perf_addr_filter documentation:
> -                *      @range: 1: range, 0: address
> -                *
>                  * At this time we don't allow range and start/stop filtering
>                  * to cohabitate, they have to be mutually exclusive.
>                  */
> -               if ((filter->range == 1) && address)
> +               if (range && address)
>                         return -EOPNOTSUPP;
> -
> -               if ((filter->range == 0) && range)
> -                       return -EOPNOTSUPP;
> -
> -               /*
> -                * For range filtering, the second address in the address
> -                * range comparator needs to be higher than the first.
> -                * Invalid otherwise.
> -                */
> -               if (filter->range && filter->size == 0)
> -                       return -EINVAL;

I'm good with that as this check is now done in perf_event_parse_addr_filter().

> -
> -               /*
> -                * Everything checks out with this filter, record what we've
> -                * received before moving on to the next one.
> -                */
> -               if (filter->range)
> -                       range = true;
> -               else
> -                       address = true;
>         }
>
>         return 0;
> @@ -440,18 +431,20 @@ static void etm_addr_filters_sync(struct perf_event *event)
>                 stop = start + filter->size;
>                 etm_filter = &filters->etm_filter[i];
>
> -               if (filter->range == 1) {
> +               switch (filter->action) {
> +               case PERF_ADDR_FILTER_ACTION_FILTER:
>                         etm_filter->start_addr = start;
>                         etm_filter->stop_addr = stop;
>                         etm_filter->type = ETM_ADDR_TYPE_RANGE;
> -               } else {
> -                       if (filter->filter == 1) {
> -                               etm_filter->start_addr = start;
> -                               etm_filter->type = ETM_ADDR_TYPE_START;
> -                       } else {
> -                               etm_filter->stop_addr = stop;
> -                               etm_filter->type = ETM_ADDR_TYPE_STOP;
> -                       }
> +                       break;
> +               case PERF_ADDR_FILTER_ACTION_START:
> +                       etm_filter->start_addr = start;
> +                       etm_filter->type = ETM_ADDR_TYPE_START;
> +                       break;
> +               case PERF_ADDR_FILTER_ACTION_STOP:
> +                       etm_filter->stop_addr = stop;
> +                       etm_filter->type = ETM_ADDR_TYPE_STOP;
> +                       break;

That is correct and reflects the current implementation.

>                 }
>                 i++;
>         }
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 1432df5a82..c09aff84f7 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -457,14 +457,19 @@ struct pmu {
>         int (*filter_match)             (struct perf_event *event); /* optional */
>  };
>
> +enum perf_addr_filter_action_t {
> +       PERF_ADDR_FILTER_ACTION_STOP = 0,
> +       PERF_ADDR_FILTER_ACTION_START,
> +       PERF_ADDR_FILTER_ACTION_FILTER,
> +};
> +
>  /**
>   * struct perf_addr_filter - address range filter definition
>   * @entry:     event's filter list linkage
>   * @inode:     object file's inode for file-based filters
>   * @offset:    filter range offset
> - * @size:      filter range size
> - * @range:     1: range, 0: address
> - * @filter:    1: filter/start, 0: stop
> + * @size:      filter range size (size==0 means single address trigger)
> + * @action:    filter/start/stop
>   *
>   * This is a hardware-agnostic filter configuration as specified by the user.
>   */
> @@ -473,8 +478,7 @@ struct perf_addr_filter {
>         struct inode            *inode;
>         unsigned long           offset;
>         unsigned long           size;
> -       unsigned int            range   : 1,
> -                               filter  : 1;
> +       enum perf_addr_filter_action_t  action;
>  };
>
>  /**
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 39106ae61b..d7a11faac1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8194,7 +8194,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>   *  * for kernel addresses: <start address>[/<size>]
>   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
>   *
> - * if <size> is not specified, the range is treated as a single address.
> + * if <size> is not specified or is zero, the range is treated as a single
> + * address; not valid for ACTION=="filter".

Now that a size of 0 can't be specified with a "filter" action, I'm
good with that statement.

Thanks,
Mathieu

>   */
>  enum {
>         IF_ACT_NONE = -1,
> @@ -8244,6 +8245,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                 return -ENOMEM;
>
>         while ((start = strsep(&fstr, " ,\n")) != NULL) {
> +               static const enum perf_addr_filter_action_t actions[] = {
> +                       [IF_ACT_FILTER] = PERF_ADDR_FILTER_ACTION_FILTER,
> +                       [IF_ACT_START]  = PERF_ADDR_FILTER_ACTION_START,
> +                       [IF_ACT_STOP]   = PERF_ADDR_FILTER_ACTION_STOP,
> +               };
>                 ret = -EINVAL;
>
>                 if (!*start)
> @@ -8260,12 +8266,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                 switch (token) {
>                 case IF_ACT_FILTER:
>                 case IF_ACT_START:
> -                       filter->filter = 1;
> -
>                 case IF_ACT_STOP:
>                         if (state != IF_STATE_ACTION)
>                                 goto fail;
>
> +                       filter->action = actions[token];
>                         state = IF_STATE_SOURCE;
>                         break;
>
> @@ -8278,15 +8283,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         if (state != IF_STATE_SOURCE)
>                                 goto fail;
>
> -                       if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
> -                               filter->range = 1;
> -
>                         *args[0].to = 0;
>                         ret = kstrtoul(args[0].from, 0, &filter->offset);
>                         if (ret)
>                                 goto fail;
>
> -                       if (filter->range) {
> +                       if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
>                                 *args[1].to = 0;
>                                 ret = kstrtoul(args[1].from, 0, &filter->size);
>                                 if (ret)
> @@ -8294,7 +8296,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         }
>
>                         if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
> -                               int fpos = filter->range ? 2 : 1;
> +                               int fpos = token == IF_SRC_FILE ? 2 : 1;
>
>                                 filename = match_strdup(&args[fpos]);
>                                 if (!filename) {
> @@ -8319,6 +8321,14 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         if (kernel && event->attr.exclude_kernel)
>                                 goto fail;
>
> +                       /*
> +                        * ACTION "filter" must have a non-zero length region
> +                        * specified.
> +                        */
> +                       if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER &&
> +                           !filter->size)
> +                               goto fail;
> +
>                         if (!kernel) {
>                                 if (!filename)
>                                         goto fail;
> --
> 2.11.0
>

Re: [PATCH 3/3] perf: Allow kernel filters on cpu events

[Ingo Molnar]

* Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> While supporting file-based address filters for cpu events requires some
> extra context switch handling, kernel address filters are easy, since the
> kernel mapping is preserved across address spaces. It is also useful as
> it permits tracing scheduling paths of the kernel.
> 
> This patch allows setting up kernel filters for cpu events.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  include/linux/perf_event.h |  2 ++
>  kernel/events/core.c       | 42 ++++++++++++++++++++++++++++--------------
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index fcb37c81ca..f4ea0600b2 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -486,6 +486,7 @@ struct perf_addr_filter {
>   * @list:	list of filters for this event
>   * @lock:	spinlock that serializes accesses to the @list and event's
>   *		(and its children's) filter generations.
> + * @nr_file_filters:	number of file-based filters

I've applied the patch, but please for heaven's sake, when you add such a long, 
long argument name, at minimum re-tabulate the documentation section to look good 
again ...

Thanks,

	Ingo

[tip:perf/core] perf/core: Do error out on a kernel filter on an exclude_filter event

[tip-bot for Alexander Shishkin]

Commit-ID:  9ccbfbb157a38921702402281ca7be530b4c3669
Gitweb:     http://git.kernel.org/tip/9ccbfbb157a38921702402281ca7be530b4c3669
Author:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate: Thu, 26 Jan 2017 11:40:56 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 10 Feb 2017 09:08:09 +0100

perf/core: Do error out on a kernel filter on an exclude_filter event

It is currently possible to configure a kernel address filter for a
event that excludes kernel from its traces (attr.exclude_kernel==1).

While in reality this doesn't make sense, the SET_FILTER ioctl() should
return a error in such case, currently it does not. Furthermore, it
will still silently discard the filter and any potentially valid filters
that came with it.

This patch makes the SET_FILTER ioctl() error out in such cases.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: Will Deacon <will.deacon@arm.com>
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20170126094057.13805-3-alexander.shishkin@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 88676ff..1730995 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8260,6 +8260,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 		 * attribute.
 		 */
 		if (state == IF_STATE_END) {
+			ret = -EINVAL;
 			if (kernel && event->attr.exclude_kernel)
 				goto fail;
 

[tip:perf/core] perf/core: Allow kernel filters on CPU events

[tip-bot for Alexander Shishkin]

Commit-ID:  6ce77bfd6cedbff61eabf8837dc0901bb671cc86
Gitweb:     http://git.kernel.org/tip/6ce77bfd6cedbff61eabf8837dc0901bb671cc86
Author:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate: Thu, 26 Jan 2017 11:40:57 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 10 Feb 2017 09:08:09 +0100

perf/core: Allow kernel filters on CPU events

While supporting file-based address filters for CPU events requires some
extra context switch handling, kernel address filters are easy, since the
kernel mapping is preserved across address spaces. It is also useful as
it permits tracing scheduling paths of the kernel.

This patch allows setting up kernel filters for CPU events.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: Will Deacon <will.deacon@arm.com>
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20170126094057.13805-4-alexander.shishkin@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/perf_event.h |  2 ++
 kernel/events/core.c       | 42 ++++++++++++++++++++++++++++--------------
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 5c58e93..000fdb2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -482,6 +482,7 @@ struct perf_addr_filter {
  * @list:	list of filters for this event
  * @lock:	spinlock that serializes accesses to the @list and event's
  *		(and its children's) filter generations.
+ * @nr_file_filters:	number of file-based filters
  *
  * A child event will use parent's @list (and therefore @lock), so they are
  * bundled together; see perf_event_addr_filters().
@@ -489,6 +490,7 @@ struct perf_addr_filter {
 struct perf_addr_filters_head {
 	struct list_head	list;
 	raw_spinlock_t		lock;
+	unsigned int		nr_file_filters;
 };
 
 /**
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1730995..a866424 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8090,6 +8090,9 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
 	if (task == TASK_TOMBSTONE)
 		return;
 
+	if (!ifh->nr_file_filters)
+		return;
+
 	mm = get_task_mm(event->ctx->task);
 	if (!mm)
 		goto restart;
@@ -8268,6 +8271,18 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 				if (!filename)
 					goto fail;
 
+				/*
+				 * For now, we only support file-based filters
+				 * in per-task events; doing so for CPU-wide
+				 * events requires additional context switching
+				 * trickery, since same object code will be
+				 * mapped at different virtual addresses in
+				 * different processes.
+				 */
+				ret = -EOPNOTSUPP;
+				if (!event->ctx->task)
+					goto fail_free_name;
+
 				/* look up the path and grab its inode */
 				ret = kern_path(filename, LOOKUP_FOLLOW, &path);
 				if (ret)
@@ -8283,6 +8298,8 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 				    !S_ISREG(filter->inode->i_mode))
 					/* free_filters_list() will iput() */
 					goto fail;
+
+				event->addr_filters.nr_file_filters++;
 			}
 
 			/* ready to consume more filters */
@@ -8322,24 +8339,13 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str)
 	if (WARN_ON_ONCE(event->parent))
 		return -EINVAL;
 
-	/*
-	 * For now, we only support filtering in per-task events; doing so
-	 * for CPU-wide events requires additional context switching trickery,
-	 * since same object code will be mapped at different virtual
-	 * addresses in different processes.
-	 */
-	if (!event->ctx->task)
-		return -EOPNOTSUPP;
-
 	ret = perf_event_parse_addr_filter(event, filter_str, &filters);
 	if (ret)
-		return ret;
+		goto fail_clear_files;
 
 	ret = event->pmu->addr_filters_validate(&filters);
-	if (ret) {
-		free_filters_list(&filters);
-		return ret;
-	}
+	if (ret)
+		goto fail_free_filters;
 
 	/* remove existing filters, if any */
 	perf_addr_filters_splice(event, &filters);
@@ -8348,6 +8354,14 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str)
 	perf_event_for_each_child(event, perf_event_addr_filters_apply);
 
 	return ret;
+
+fail_free_filters:
+	free_filters_list(&filters);
+
+fail_clear_files:
+	event->addr_filters.nr_file_filters = 0;
+
+	return ret;
 }
 
 static int perf_event_set_filter(struct perf_event *event, void __user *arg)

Re: [PATCH 3/3] perf: Allow kernel filters on cpu events

[Alexander Shishkin]

Ingo Molnar <mingo@kernel.org> writes:

> * Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:
>
>> While supporting file-based address filters for cpu events requires some
>> extra context switch handling, kernel address filters are easy, since the
>> kernel mapping is preserved across address spaces. It is also useful as
>> it permits tracing scheduling paths of the kernel.
>> 
>> This patch allows setting up kernel filters for cpu events.
>> 
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  include/linux/perf_event.h |  2 ++
>>  kernel/events/core.c       | 42 ++++++++++++++++++++++++++++--------------
>>  2 files changed, 30 insertions(+), 14 deletions(-)
>> 
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index fcb37c81ca..f4ea0600b2 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -486,6 +486,7 @@ struct perf_addr_filter {
>>   * @list:	list of filters for this event
>>   * @lock:	spinlock that serializes accesses to the @list and event's
>>   *		(and its children's) filter generations.
>> + * @nr_file_filters:	number of file-based filters
>
> I've applied the patch, but please for heaven's sake, when you add such a long, 
> long argument name, at minimum re-tabulate the documentation section to look good 
> again ...

Will do.

>From 8b993fa566acb0cd333ce5e433f1e6858b181f58 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Tue, 14 Feb 2017 14:49:41 +0200
Subject: [PATCH] perf: Fix alignment in a kerneldoc comment

Commit 6ce77bfd6c ("perf/core: Allow kernel filters on CPU events") added
"nr_file_filters" to the filters structure and its kerneldoc description,
which now requires that other fields' descriptions be padded to the next
tabstop to not look ugly.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 258ebdb533..8f347b84cf 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -503,9 +503,9 @@ struct perf_addr_filter {
 
 /**
  * struct perf_addr_filters_head - container for address range filters
- * @list:	list of filters for this event
- * @lock:	spinlock that serializes accesses to the @list and event's
- *		(and its children's) filter generations.
+ * @list:		list of filters for this event
+ * @lock:		spinlock that serializes accesses to the @list and
+ *			event's (and its children's) filter generations.
  * @nr_file_filters:	number of file-based filters
  *
  * A child event will use parent's @list (and therefore @lock), so they are
-- 
2.11.0

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

On 17 January 2018 at 05:31, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> On Tue, Feb 07, 2017 at 10:50:50AM -0700, Mathieu Poirier wrote:
>> > index 39106ae61b..d7a11faac1 100644
>> > --- a/kernel/events/core.c
>> > +++ b/kernel/events/core.c
>> > @@ -8194,7 +8194,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>> >   *  * for kernel addresses: <start address>[/<size>]
>> >   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
>> >   *
>> > - * if <size> is not specified, the range is treated as a single address.
>> > + * if <size> is not specified or is zero, the range is treated as a single
>> > + * address; not valid for ACTION=="filter".
>>
>> Now that a size of 0 can't be specified with a "filter" action, I'm
>> good with that statement.
>
> Hi Mathieu, I completely lost track of this.
>
> Following is the commit I found dangilng in one of my local branches.
> Does this make sense to you? Thanks!

Oh boy!  That's a whole year ago...  Give me some time to wrap my
brain around it again.

Thanks,
Mathieu

>
> From 031b37dc50cb9bde86f97351edd3085b5f983ce7 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Mon, 23 Jan 2017 17:29:20 +0200
> Subject: [PATCH] perf, pt, coresight: Clean up address filter structure
>
> This is a cosmetic patch that deals with the address filter structure's
> ambiguous fields 'filter' and 'range'. The former stands to mean that the
> filter's *action* should be to filter the traces to its address range if
> it's set or stop tracing if it's unset. This is confusing and hard on the
> eyes, so this patch replaces it with 'action' enum. The 'range' field is
> completely redundant (meaning that the filter is an address range as
> opposed to a single address trigger), as we can use zero size to mean the
> same thing.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  arch/x86/events/intel/pt.c                       | 13 ++++--
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 59 +++++++++++-------------
>  include/linux/perf_event.h                       | 14 ++++--
>  kernel/events/core.c                             | 26 +++++++----
>  4 files changed, 63 insertions(+), 49 deletions(-)
>
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 81fd41d5a0d9..3b993942a0e4 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1186,8 +1186,12 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
>         int range = 0;
>
>         list_for_each_entry(filter, filters, entry) {
> -               /* PT doesn't support single address triggers */
> -               if (!filter->range || !filter->size)
> +               /*
> +                * PT doesn't support single address triggers and
> +                * 'start' filters.
> +                */
> +               if (!filter->size ||
> +                   filter->action == PERF_ADDR_FILTER_ACTION_START)
>                         return -EOPNOTSUPP;
>
>                 if (!filter->inode) {
> @@ -1227,7 +1231,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
>
>                 filters->filter[range].msr_a  = msr_a;
>                 filters->filter[range].msr_b  = msr_b;
> -               filters->filter[range].config = filter->filter ? 1 : 2;
> +               if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
> +                       filters->filter[range].config = 1;
> +               else
> +                       filters->filter[range].config = 2;
>                 range++;
>         }
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 8a0ad77574e7..4e5ed6597f2f 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -393,35 +393,26 @@ static int etm_addr_filters_validate(struct list_head *filters)
>                 if (++index > ETM_ADDR_CMP_MAX)
>                         return -EOPNOTSUPP;
>
> +               /* filter::size==0 means single address trigger */
> +               if (filter->size) {
> +                       /*
> +                        * The existing code relies on START/STOP filters
> +                        * being address filters.
> +                        */
> +                       if (filter->action == PERF_ADDR_FILTER_ACTION_START ||
> +                           filter->action == PERF_ADDR_FILTER_ACTION_STOP)
> +                               return -EOPNOTSUPP;
> +
> +                       range = true;
> +               } else
> +                       address = true;
> +
>                 /*
> -                * As taken from the struct perf_addr_filter documentation:
> -                *      @range: 1: range, 0: address
> -                *
>                  * At this time we don't allow range and start/stop filtering
>                  * to cohabitate, they have to be mutually exclusive.
>                  */
> -               if ((filter->range == 1) && address)
> +               if (range && address)
>                         return -EOPNOTSUPP;
> -
> -               if ((filter->range == 0) && range)
> -                       return -EOPNOTSUPP;
> -
> -               /*
> -                * For range filtering, the second address in the address
> -                * range comparator needs to be higher than the first.
> -                * Invalid otherwise.
> -                */
> -               if (filter->range && filter->size == 0)
> -                       return -EINVAL;
> -
> -               /*
> -                * Everything checks out with this filter, record what we've
> -                * received before moving on to the next one.
> -                */
> -               if (filter->range)
> -                       range = true;
> -               else
> -                       address = true;
>         }
>
>         return 0;
> @@ -441,18 +432,20 @@ static void etm_addr_filters_sync(struct perf_event *event)
>                 stop = start + filter->size;
>                 etm_filter = &filters->etm_filter[i];
>
> -               if (filter->range == 1) {
> +               switch (filter->action) {
> +               case PERF_ADDR_FILTER_ACTION_FILTER:
>                         etm_filter->start_addr = start;
>                         etm_filter->stop_addr = stop;
>                         etm_filter->type = ETM_ADDR_TYPE_RANGE;
> -               } else {
> -                       if (filter->filter == 1) {
> -                               etm_filter->start_addr = start;
> -                               etm_filter->type = ETM_ADDR_TYPE_START;
> -                       } else {
> -                               etm_filter->stop_addr = stop;
> -                               etm_filter->type = ETM_ADDR_TYPE_STOP;
> -                       }
> +                       break;
> +               case PERF_ADDR_FILTER_ACTION_START:
> +                       etm_filter->start_addr = start;
> +                       etm_filter->type = ETM_ADDR_TYPE_START;
> +                       break;
> +               case PERF_ADDR_FILTER_ACTION_STOP:
> +                       etm_filter->stop_addr = stop;
> +                       etm_filter->type = ETM_ADDR_TYPE_STOP;
> +                       break;
>                 }
>                 i++;
>         }
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 7546822a1d74..674c71fe4999 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -449,14 +449,19 @@ struct pmu {
>         int (*filter_match)             (struct perf_event *event); /* optional */
>  };
>
> +enum perf_addr_filter_action_t {
> +       PERF_ADDR_FILTER_ACTION_STOP = 0,
> +       PERF_ADDR_FILTER_ACTION_START,
> +       PERF_ADDR_FILTER_ACTION_FILTER,
> +};
> +
>  /**
>   * struct perf_addr_filter - address range filter definition
>   * @entry:     event's filter list linkage
>   * @inode:     object file's inode for file-based filters
>   * @offset:    filter range offset
> - * @size:      filter range size
> - * @range:     1: range, 0: address
> - * @filter:    1: filter/start, 0: stop
> + * @size:      filter range size (size==0 means single address trigger)
> + * @action:    filter/start/stop
>   *
>   * This is a hardware-agnostic filter configuration as specified by the user.
>   */
> @@ -465,8 +470,7 @@ struct perf_addr_filter {
>         struct inode            *inode;
>         unsigned long           offset;
>         unsigned long           size;
> -       unsigned int            range   : 1,
> -                               filter  : 1;
> +       enum perf_addr_filter_action_t  action;
>  };
>
>  /**
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4e1a1bf8d867..0a3a33519700 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8303,7 +8303,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>   *  * for kernel addresses: <start address>[/<size>]
>   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
>   *
> - * if <size> is not specified, the range is treated as a single address.
> + * if <size> is not specified or is zero, the range is treated as a single
> + * address; not valid for ACTION=="filter".
>   */
>  enum {
>         IF_ACT_NONE = -1,
> @@ -8353,6 +8354,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                 return -ENOMEM;
>
>         while ((start = strsep(&fstr, " ,\n")) != NULL) {
> +               static const enum perf_addr_filter_action_t actions[] = {
> +                       [IF_ACT_FILTER] = PERF_ADDR_FILTER_ACTION_FILTER,
> +                       [IF_ACT_START]  = PERF_ADDR_FILTER_ACTION_START,
> +                       [IF_ACT_STOP]   = PERF_ADDR_FILTER_ACTION_STOP,
> +               };
>                 ret = -EINVAL;
>
>                 if (!*start)
> @@ -8369,12 +8375,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                 switch (token) {
>                 case IF_ACT_FILTER:
>                 case IF_ACT_START:
> -                       filter->filter = 1;
> -
>                 case IF_ACT_STOP:
>                         if (state != IF_STATE_ACTION)
>                                 goto fail;
>
> +                       filter->action = actions[token];
>                         state = IF_STATE_SOURCE;
>                         break;
>
> @@ -8387,15 +8392,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         if (state != IF_STATE_SOURCE)
>                                 goto fail;
>
> -                       if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
> -                               filter->range = 1;
> -
>                         *args[0].to = 0;
>                         ret = kstrtoul(args[0].from, 0, &filter->offset);
>                         if (ret)
>                                 goto fail;
>
> -                       if (filter->range) {
> +                       if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
>                                 *args[1].to = 0;
>                                 ret = kstrtoul(args[1].from, 0, &filter->size);
>                                 if (ret)
> @@ -8403,7 +8405,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         }
>
>                         if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
> -                               int fpos = filter->range ? 2 : 1;
> +                               int fpos = token == IF_SRC_FILE ? 2 : 1;
>
>                                 filename = match_strdup(&args[fpos]);
>                                 if (!filename) {
> @@ -8429,6 +8431,14 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         if (kernel && event->attr.exclude_kernel)
>                                 goto fail;
>
> +                       /*
> +                        * ACTION "filter" must have a non-zero length region
> +                        * specified.
> +                        */
> +                       if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER &&
> +                           !filter->size)
> +                               goto fail;
> +
>                         if (!kernel) {
>                                 if (!filename)
>                                         goto fail;
> --
> 2.15.1
>

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Will Deacon]

On Thu, Jan 18, 2018 at 09:59:26AM -0700, Mathieu Poirier wrote:
> On 17 January 2018 at 05:31, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
> > On Tue, Feb 07, 2017 at 10:50:50AM -0700, Mathieu Poirier wrote:
> >> > index 39106ae61b..d7a11faac1 100644
> >> > --- a/kernel/events/core.c
> >> > +++ b/kernel/events/core.c
> >> > @@ -8194,7 +8194,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
> >> >   *  * for kernel addresses: <start address>[/<size>]
> >> >   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
> >> >   *
> >> > - * if <size> is not specified, the range is treated as a single address.
> >> > + * if <size> is not specified or is zero, the range is treated as a single
> >> > + * address; not valid for ACTION=="filter".
> >>
> >> Now that a size of 0 can't be specified with a "filter" action, I'm
> >> good with that statement.
> >
> > Hi Mathieu, I completely lost track of this.
> >
> > Following is the commit I found dangilng in one of my local branches.
> > Does this make sense to you? Thanks!
> 
> Oh boy!  That's a whole year ago...  Give me some time to wrap my
> brain around it again.

Do we need anything for SPE, or is this only applicable to certain types of
tracing PMUs?

Will

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

On 18 January 2018 at 10:06, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jan 18, 2018 at 09:59:26AM -0700, Mathieu Poirier wrote:
>> On 17 January 2018 at 05:31, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>> > On Tue, Feb 07, 2017 at 10:50:50AM -0700, Mathieu Poirier wrote:
>> >> > index 39106ae61b..d7a11faac1 100644
>> >> > --- a/kernel/events/core.c
>> >> > +++ b/kernel/events/core.c
>> >> > @@ -8194,7 +8194,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>> >> >   *  * for kernel addresses: <start address>[/<size>]
>> >> >   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
>> >> >   *
>> >> > - * if <size> is not specified, the range is treated as a single address.
>> >> > + * if <size> is not specified or is zero, the range is treated as a single
>> >> > + * address; not valid for ACTION=="filter".
>> >>
>> >> Now that a size of 0 can't be specified with a "filter" action, I'm
>> >> good with that statement.
>> >
>> > Hi Mathieu, I completely lost track of this.
>> >
>> > Following is the commit I found dangilng in one of my local branches.
>> > Does this make sense to you? Thanks!
>>
>> Oh boy!  That's a whole year ago...  Give me some time to wrap my
>> brain around it again.
>
> Do we need anything for SPE, or is this only applicable to certain types of
> tracing PMUs?

As far as I can tell spe_pmu->pmu->nr_addr_filters isn't set anywhere.
A such SPE isn't concerned here.

Mathieu

>
> Will

Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Mathieu Poirier]

On 17 January 2018 at 05:31, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> On Tue, Feb 07, 2017 at 10:50:50AM -0700, Mathieu Poirier wrote:
>> > index 39106ae61b..d7a11faac1 100644
>> > --- a/kernel/events/core.c
>> > +++ b/kernel/events/core.c
>> > @@ -8194,7 +8194,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>> >   *  * for kernel addresses: <start address>[/<size>]
>> >   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
>> >   *
>> > - * if <size> is not specified, the range is treated as a single address.
>> > + * if <size> is not specified or is zero, the range is treated as a single
>> > + * address; not valid for ACTION=="filter".
>>
>> Now that a size of 0 can't be specified with a "filter" action, I'm
>> good with that statement.
>
> Hi Mathieu, I completely lost track of this.
>
> Following is the commit I found dangilng in one of my local branches.
> Does this make sense to you? Thanks!
>

Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> From 031b37dc50cb9bde86f97351edd3085b5f983ce7 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Mon, 23 Jan 2017 17:29:20 +0200
> Subject: [PATCH] perf, pt, coresight: Clean up address filter structure
>
> This is a cosmetic patch that deals with the address filter structure's
> ambiguous fields 'filter' and 'range'. The former stands to mean that the
> filter's *action* should be to filter the traces to its address range if
> it's set or stop tracing if it's unset. This is confusing and hard on the
> eyes, so this patch replaces it with 'action' enum. The 'range' field is
> completely redundant (meaning that the filter is an address range as
> opposed to a single address trigger), as we can use zero size to mean the
> same thing.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  arch/x86/events/intel/pt.c                       | 13 ++++--
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 59 +++++++++++-------------
>  include/linux/perf_event.h                       | 14 ++++--
>  kernel/events/core.c                             | 26 +++++++----
>  4 files changed, 63 insertions(+), 49 deletions(-)
>
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 81fd41d5a0d9..3b993942a0e4 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1186,8 +1186,12 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
>         int range = 0;
>
>         list_for_each_entry(filter, filters, entry) {
> -               /* PT doesn't support single address triggers */
> -               if (!filter->range || !filter->size)
> +               /*
> +                * PT doesn't support single address triggers and
> +                * 'start' filters.
> +                */
> +               if (!filter->size ||
> +                   filter->action == PERF_ADDR_FILTER_ACTION_START)
>                         return -EOPNOTSUPP;
>
>                 if (!filter->inode) {
> @@ -1227,7 +1231,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
>
>                 filters->filter[range].msr_a  = msr_a;
>                 filters->filter[range].msr_b  = msr_b;
> -               filters->filter[range].config = filter->filter ? 1 : 2;
> +               if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
> +                       filters->filter[range].config = 1;
> +               else
> +                       filters->filter[range].config = 2;
>                 range++;
>         }
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 8a0ad77574e7..4e5ed6597f2f 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -393,35 +393,26 @@ static int etm_addr_filters_validate(struct list_head *filters)
>                 if (++index > ETM_ADDR_CMP_MAX)
>                         return -EOPNOTSUPP;
>
> +               /* filter::size==0 means single address trigger */
> +               if (filter->size) {
> +                       /*
> +                        * The existing code relies on START/STOP filters
> +                        * being address filters.
> +                        */
> +                       if (filter->action == PERF_ADDR_FILTER_ACTION_START ||
> +                           filter->action == PERF_ADDR_FILTER_ACTION_STOP)
> +                               return -EOPNOTSUPP;
> +
> +                       range = true;
> +               } else
> +                       address = true;
> +
>                 /*
> -                * As taken from the struct perf_addr_filter documentation:
> -                *      @range: 1: range, 0: address
> -                *
>                  * At this time we don't allow range and start/stop filtering
>                  * to cohabitate, they have to be mutually exclusive.
>                  */
> -               if ((filter->range == 1) && address)
> +               if (range && address)
>                         return -EOPNOTSUPP;
> -
> -               if ((filter->range == 0) && range)
> -                       return -EOPNOTSUPP;
> -
> -               /*
> -                * For range filtering, the second address in the address
> -                * range comparator needs to be higher than the first.
> -                * Invalid otherwise.
> -                */
> -               if (filter->range && filter->size == 0)
> -                       return -EINVAL;
> -
> -               /*
> -                * Everything checks out with this filter, record what we've
> -                * received before moving on to the next one.
> -                */
> -               if (filter->range)
> -                       range = true;
> -               else
> -                       address = true;
>         }
>
>         return 0;
> @@ -441,18 +432,20 @@ static void etm_addr_filters_sync(struct perf_event *event)
>                 stop = start + filter->size;
>                 etm_filter = &filters->etm_filter[i];
>
> -               if (filter->range == 1) {
> +               switch (filter->action) {
> +               case PERF_ADDR_FILTER_ACTION_FILTER:
>                         etm_filter->start_addr = start;
>                         etm_filter->stop_addr = stop;
>                         etm_filter->type = ETM_ADDR_TYPE_RANGE;
> -               } else {
> -                       if (filter->filter == 1) {
> -                               etm_filter->start_addr = start;
> -                               etm_filter->type = ETM_ADDR_TYPE_START;
> -                       } else {
> -                               etm_filter->stop_addr = stop;
> -                               etm_filter->type = ETM_ADDR_TYPE_STOP;
> -                       }
> +                       break;
> +               case PERF_ADDR_FILTER_ACTION_START:
> +                       etm_filter->start_addr = start;
> +                       etm_filter->type = ETM_ADDR_TYPE_START;
> +                       break;
> +               case PERF_ADDR_FILTER_ACTION_STOP:
> +                       etm_filter->stop_addr = stop;
> +                       etm_filter->type = ETM_ADDR_TYPE_STOP;
> +                       break;
>                 }
>                 i++;
>         }
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 7546822a1d74..674c71fe4999 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -449,14 +449,19 @@ struct pmu {
>         int (*filter_match)             (struct perf_event *event); /* optional */
>  };
>
> +enum perf_addr_filter_action_t {
> +       PERF_ADDR_FILTER_ACTION_STOP = 0,
> +       PERF_ADDR_FILTER_ACTION_START,
> +       PERF_ADDR_FILTER_ACTION_FILTER,
> +};
> +
>  /**
>   * struct perf_addr_filter - address range filter definition
>   * @entry:     event's filter list linkage
>   * @inode:     object file's inode for file-based filters
>   * @offset:    filter range offset
> - * @size:      filter range size
> - * @range:     1: range, 0: address
> - * @filter:    1: filter/start, 0: stop
> + * @size:      filter range size (size==0 means single address trigger)
> + * @action:    filter/start/stop
>   *
>   * This is a hardware-agnostic filter configuration as specified by the user.
>   */
> @@ -465,8 +470,7 @@ struct perf_addr_filter {
>         struct inode            *inode;
>         unsigned long           offset;
>         unsigned long           size;
> -       unsigned int            range   : 1,
> -                               filter  : 1;
> +       enum perf_addr_filter_action_t  action;
>  };
>
>  /**
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4e1a1bf8d867..0a3a33519700 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8303,7 +8303,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
>   *  * for kernel addresses: <start address>[/<size>]
>   *  * for object files:     <start address>[/<size>]@</path/to/object/file>
>   *
> - * if <size> is not specified, the range is treated as a single address.
> + * if <size> is not specified or is zero, the range is treated as a single
> + * address; not valid for ACTION=="filter".
>   */
>  enum {
>         IF_ACT_NONE = -1,
> @@ -8353,6 +8354,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                 return -ENOMEM;
>
>         while ((start = strsep(&fstr, " ,\n")) != NULL) {
> +               static const enum perf_addr_filter_action_t actions[] = {
> +                       [IF_ACT_FILTER] = PERF_ADDR_FILTER_ACTION_FILTER,
> +                       [IF_ACT_START]  = PERF_ADDR_FILTER_ACTION_START,
> +                       [IF_ACT_STOP]   = PERF_ADDR_FILTER_ACTION_STOP,
> +               };
>                 ret = -EINVAL;
>
>                 if (!*start)
> @@ -8369,12 +8375,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                 switch (token) {
>                 case IF_ACT_FILTER:
>                 case IF_ACT_START:
> -                       filter->filter = 1;
> -
>                 case IF_ACT_STOP:
>                         if (state != IF_STATE_ACTION)
>                                 goto fail;
>
> +                       filter->action = actions[token];
>                         state = IF_STATE_SOURCE;
>                         break;
>
> @@ -8387,15 +8392,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         if (state != IF_STATE_SOURCE)
>                                 goto fail;
>
> -                       if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
> -                               filter->range = 1;
> -
>                         *args[0].to = 0;
>                         ret = kstrtoul(args[0].from, 0, &filter->offset);
>                         if (ret)
>                                 goto fail;
>
> -                       if (filter->range) {
> +                       if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
>                                 *args[1].to = 0;
>                                 ret = kstrtoul(args[1].from, 0, &filter->size);
>                                 if (ret)
> @@ -8403,7 +8405,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         }
>
>                         if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
> -                               int fpos = filter->range ? 2 : 1;
> +                               int fpos = token == IF_SRC_FILE ? 2 : 1;
>
>                                 filename = match_strdup(&args[fpos]);
>                                 if (!filename) {
> @@ -8429,6 +8431,14 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>                         if (kernel && event->attr.exclude_kernel)
>                                 goto fail;
>
> +                       /*
> +                        * ACTION "filter" must have a non-zero length region
> +                        * specified.
> +                        */
> +                       if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER &&
> +                           !filter->size)
> +                               goto fail;
> +
>                         if (!kernel) {
>                                 if (!filename)
>                                         goto fail;
> --
> 2.15.1
>