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

From: Mathieu Poirier
Date: Fri Jan 19 2018 - 13:50:35 EST

Next message: Andrew Lunn: "Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support"
Previous message: David Miller: "Re: [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail"
In reply to: Mathieu Poirier: "Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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!
>

Acked-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

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

Next message: Andrew Lunn: "Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support"
Previous message: David Miller: "Re: [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail"
In reply to: Mathieu Poirier: "Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]