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

Groups > linux.kernel > #1571538 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-02-01 13:50 +0100
    Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-02-01 22:40 +0100
      Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-02-01 23:20 +0100
        Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-02-02 11:50 +0100
          Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-02-02 18:40 +0100
      Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-02-02 17:30 +0100
        Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Mathieu Poirier <mathieu.poirier@linaro.org> - 2017-02-07 19:00 +0100

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

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

[toc] | [next] | [standalone]

#1571994

)

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

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

#1572016

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

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

#1572266

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

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

#1572543

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

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

#1572503

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

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

#1575930

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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web