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