From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>,
Ingo Molnar <mingo@redhat.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Vince Weaver <vince@deater.net>,
Stephane Eranian <eranian@google.com>,
Arnaldo Carvalho de Melo <acme@infradead.org>,
Will Deacon <will.deacon@arm.com>,
Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure
Date: Fri, 19 Jan 2018 11:50:22 -0700 [thread overview]
Message-ID: <CANLsYkz6_0jwWkYtFf=6aZLkU2nFGL+6AAGvm4mhmXZwR8fBGA@mail.gmail.com> (raw)
In-Reply-To: <20180117123137.3hlmudzu5eogl53n@ukko.fi.intel.com>
On 17 January 2018 at 05:31, Alexander Shishkin
<alexander.shishkin@linux.intel.com> 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@linaro.org>
> From 031b37dc50cb9bde86f97351edd3085b5f983ce7 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
>
> 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@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 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 prev parent reply other threads:[~2018-01-19 18:50 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-26 9:40 [PATCH 0/3] perf: Updates for address filters Alexander Shishkin
2017-01-26 9:40 ` [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Alexander Shishkin
2017-01-26 13:02 ` kbuild test robot
2017-01-26 13:24 ` Alexander Shishkin
2017-01-26 18:26 ` Mathieu Poirier
2017-01-27 12:12 ` Alexander Shishkin
2017-01-27 17:17 ` Mathieu Poirier
2017-02-01 12:46 ` Alexander Shishkin
2017-02-01 21:33 ` Mathieu Poirier
2017-02-01 22:15 ` Mathieu Poirier
2017-02-02 10:42 ` Alexander Shishkin
2017-02-02 17:36 ` Mathieu Poirier
2017-02-02 16:22 ` Alexander Shishkin
2017-02-07 17:50 ` Mathieu Poirier
[not found] ` <20180117123137.3hlmudzu5eogl53n@ukko.fi.intel.com>
2018-01-18 16:59 ` Mathieu Poirier
2018-01-18 17:06 ` Will Deacon
2018-01-18 18:19 ` Mathieu Poirier
2018-01-19 18:50 ` Mathieu Poirier [this message]
2017-01-26 9:40 ` [PATCH 2/3] perf: Do error out on a kernel filter on an exclude_filter event Alexander Shishkin
2017-01-26 18:32 ` Mathieu Poirier
2017-02-10 8:33 ` [tip:perf/core] perf/core: " tip-bot for Alexander Shishkin
2017-01-26 9:40 ` [PATCH 3/3] perf: Allow kernel filters on cpu events Alexander Shishkin
2017-01-26 21:38 ` Mathieu Poirier
2017-01-27 12:31 ` Alexander Shishkin
2017-01-27 17:38 ` Mathieu Poirier
2017-02-10 8:07 ` Ingo Molnar
2017-02-14 12:59 ` Alexander Shishkin
2017-02-10 8:34 ` [tip:perf/core] perf/core: Allow kernel filters on CPU events tip-bot for Alexander Shishkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANLsYkz6_0jwWkYtFf=6aZLkU2nFGL+6AAGvm4mhmXZwR8fBGA@mail.gmail.com' \
--to=mathieu.poirier@linaro.org \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@infradead.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=eranian@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=vince@deater.net \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).