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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> 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@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 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
>