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@xxxxxxxxxxxxxxx>
> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> ---
>  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
>