Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567238 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] perf: Updates for address filters Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-01-26 10:50 +0100
    [PATCH 3/3] perf: Allow kernel filters on cpu events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-01-26 10:50 +0100
      Re: [PATCH 3/3] perf: Allow kernel filters on cpu events Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-01-26 22:50 +0100
        Re: [PATCH 3/3] perf: Allow kernel filters on cpu events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-01-27 13:50 +0100
          Re: [PATCH 3/3] perf: Allow kernel filters on cpu events Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-01-27 22:50 +0100
    [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-01-26 10:50 +0100
      Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter  structure Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-01-26 19:30 +0100
        Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-01-27 13:20 +0100
          Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-01-27 18:20 +0100

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

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

[toc] | [next] | [standalone]

#1567241 — [PATCH 3/3] perf: 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>
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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1567246 — [PATCH 1/3] 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 | 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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web