* [PATCH 0/3] perf: Updates for address filters
@ 2017-01-26 9:40 Alexander Shishkin
2017-01-26 9:40 ` [PATCH 1/3] perf, pt, coresight: Clean up address filter structure Alexander Shishkin
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Alexander Shishkin @ 2017-01-26 9:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, linux-kernel, vince, eranian,
Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland,
Alexander Shishkin
Hi Peter,
Here's a small update. One 'feature' that is added is kernel filters on
cpu events, so that one can trace scheduling paths and suchlike. While at
it, I also brushed up the filter structure a bit, iirc that's also what
Ingo wanted. And lastly there was one glitch in the filter parsing that
silently discarded filters when a kernel filter was SET_FILTER'ed on a
exclude_kernel==1 event.
Alexander Shishkin (3):
perf, pt, coresight: Clean up address filter structure
perf: Do error out on a kernel filter on an exclude_filter event
perf: Allow kernel filters on cpu events
arch/x86/events/intel/pt.c | 7 ++-
drivers/hwtracing/coresight/coresight-etm-perf.c | 33 ++++----------
include/linux/perf_event.h | 16 ++++---
kernel/events/core.c | 55 +++++++++++++++---------
4 files changed, 58 insertions(+), 53 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-01-26 9:40 [PATCH 0/3] perf: Updates for address filters Alexander Shishkin @ 2017-01-26 9:40 ` Alexander Shishkin 2017-01-26 13:02 ` kbuild test robot 2017-01-26 18:26 ` Mathieu Poirier 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 9:40 ` [PATCH 3/3] perf: Allow kernel filters on cpu events Alexander Shishkin 2 siblings, 2 replies; 28+ messages in thread From: Alexander Shishkin @ 2017-01-26 9:40 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland, Alexander Shishkin, Mathieu Poirier 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 | 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; - - /* - * 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, +}; + /** * 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; 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) { *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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 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 1 sibling, 1 reply; 28+ messages in thread From: kbuild test robot @ 2017-01-26 13:02 UTC (permalink / raw) To: Alexander Shishkin Cc: kbuild-all, Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland, Alexander Shishkin, Mathieu Poirier [-- Attachment #1: Type: text/plain, Size: 2205 bytes --] Hi Alexander, [auto build test ERROR on tip/perf/core] [also build test ERROR on v4.10-rc5 next-20170125] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Alexander-Shishkin/perf-Updates-for-address-filters/20170126-175256 config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All errors (new ones prefixed by >>): drivers/hwtracing/coresight/coresight-etm-perf.c: In function 'etm_addr_filters_sync': >> drivers/hwtracing/coresight/coresight-etm-perf.c:426:13: error: 'struct perf_addr_filter' has no member named 'range' if (filter->range == 1) { ^~ vim +426 drivers/hwtracing/coresight/coresight-etm-perf.c ca878b14 Mathieu Poirier 2016-08-25 420 ca878b14 Mathieu Poirier 2016-08-25 421 list_for_each_entry(filter, &head->list, entry) { ca878b14 Mathieu Poirier 2016-08-25 422 start = filter->offset + offs[i]; ca878b14 Mathieu Poirier 2016-08-25 423 stop = start + filter->size; ca878b14 Mathieu Poirier 2016-08-25 424 etm_filter = &filters->etm_filter[i]; ca878b14 Mathieu Poirier 2016-08-25 425 ca878b14 Mathieu Poirier 2016-08-25 @426 if (filter->range == 1) { ca878b14 Mathieu Poirier 2016-08-25 427 etm_filter->start_addr = start; ca878b14 Mathieu Poirier 2016-08-25 428 etm_filter->stop_addr = stop; ca878b14 Mathieu Poirier 2016-08-25 429 etm_filter->type = ETM_ADDR_TYPE_RANGE; :::::: The code at line 426 was first introduced by commit :::::: ca878b14660c340287fb17d70950cfc09c6d698c coresight: etm-perf: configuring filters from perf core :::::: TO: Mathieu Poirier <mathieu.poirier@linaro.org> :::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 60054 bytes --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-01-26 13:02 ` kbuild test robot @ 2017-01-26 13:24 ` Alexander Shishkin 0 siblings, 0 replies; 28+ messages in thread From: Alexander Shishkin @ 2017-01-26 13:24 UTC (permalink / raw) To: Peter Zijlstra Cc: kbuild-all, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland, Mathieu Poirier kbuild test robot <lkp@intel.com> writes: > Hi Alexander, > > [auto build test ERROR on tip/perf/core] > [also build test ERROR on v4.10-rc5 next-20170125] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Alexander-Shishkin/perf-Updates-for-address-filters/20170126-175256 > config: arm-allmodconfig (attached as .config) > compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 > reproduce: > wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=arm > > All errors (new ones prefixed by >>): > > drivers/hwtracing/coresight/coresight-etm-perf.c: In function 'etm_addr_filters_sync': >>> drivers/hwtracing/coresight/coresight-etm-perf.c:426:13: error: 'struct perf_addr_filter' has no member named 'range' > if (filter->range == 1) { > ^~ Right. Now it should also compile. >From f50111d892a63721bef66eae39feadcd0194b163 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 v1] 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 | 7 +++-- drivers/hwtracing/coresight/coresight-etm-perf.c | 35 ++++++------------------ include/linux/perf_event.h | 14 ++++++---- kernel/events/core.c | 12 ++++---- 4 files changed, 28 insertions(+), 40 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..f14bc5c73f 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; - - /* - * 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,12 +423,12 @@ static void etm_addr_filters_sync(struct perf_event *event) stop = start + filter->size; etm_filter = &filters->etm_filter[i]; - if (filter->range == 1) { + if (filter->size) { etm_filter->start_addr = start; 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, +}; + /** * 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; 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) { *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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 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 18:26 ` Mathieu Poirier 2017-01-27 12:12 ` Alexander Shishkin 1 sibling, 1 reply; 28+ messages in thread From: Mathieu Poirier @ 2017-01-26 18:26 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland 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@linux.intel.com> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-01-26 18:26 ` Mathieu Poirier @ 2017-01-27 12:12 ` Alexander Shishkin 2017-01-27 17:17 ` Mathieu Poirier 0 siblings, 1 reply; 28+ messages in thread From: Alexander Shishkin @ 2017-01-27 12:12 UTC (permalink / raw) To: Mathieu Poirier Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland Mathieu Poirier <mathieu.poirier@linaro.org> writes: > Hi Alex, Hi Mathieu, > 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. Not really. Currently we have 2 drivers using this and both reject the type=range&&size==0 filters with either -EOPNOTSUPP or -EINVAL. With this change, PT will still reject it as it doesn't support single address triggers, but Coresight will treat it as if it was a single address filter. Which makes sense, because that's what a range of size zero is. Note, that a range that covers one instruction has to be at least size==1 (and I'm guessing size==4 for Coresight, but I may be wrong). So yes, this does change the existing behavior, but in doing so it removes the ambiguity of zero sized ranges. > if (filter->action == PERF_ADDR_FILTER_ACTION_RANGE) But "range" is not an action, it's a type of a filter. It determines the condition that triggers an action. An action, however, is what we do when the condition comes true. Regards, -- Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-01-27 12:12 ` Alexander Shishkin @ 2017-01-27 17:17 ` Mathieu Poirier 2017-02-01 12:46 ` Alexander Shishkin 0 siblings, 1 reply; 28+ messages in thread From: Mathieu Poirier @ 2017-01-27 17:17 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland On 27 January 2017 at 05:12, Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > Mathieu Poirier <mathieu.poirier@linaro.org> writes: > >> Hi Alex, > > Hi Mathieu, > >> 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. > > Not really. Currently we have 2 drivers using this and both reject the > type=range&&size==0 filters with either -EOPNOTSUPP or -EINVAL. With > this change, PT will still reject it as it doesn't support single > address triggers, but Coresight will treat it as if it was a single > address filter. Hence my statement about a change in behaviour. > Which makes sense, because that's what a range of size > zero is. Note, that a range that covers one instruction has to be at > least size==1 (and I'm guessing size==4 for Coresight, but I may be > wrong). > > So yes, this does change the existing behavior, but in doing so it > removes the ambiguity of zero sized ranges. Specifying a size of zero with a range filter is wrong and as such should be treated as an error, which is what the current code is doing. If people want a start filter they can use the syntax required for that. In my opinion treating a range filter with a size zero as a start filter is adding intelligence to the machine, something that should probably be avoided. > >> if (filter->action == PERF_ADDR_FILTER_ACTION_RANGE) > > But "range" is not an action, it's a type of a filter. It determines the > condition that triggers an action. An action, however, is what we do > when the condition comes true. Then filter->action could be renamed 'type'. In the end filters on PT are range filters, the same way they are on CS. But changing the naming convention is a matter of personal opinion - I am fine with what we currently have. On the flip side reordering the fields in the 'if_tokens' match table would allow to set the filters properly in perf_event_parse_addr_filter(), keep the current behaviour intact and get rid of filter->range. Thanks, Mathieu > > Regards, > -- > Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-01-27 17:17 ` Mathieu Poirier @ 2017-02-01 12:46 ` Alexander Shishkin 2017-02-01 21:33 ` Mathieu Poirier 0 siblings, 1 reply; 28+ messages in thread From: Alexander Shishkin @ 2017-02-01 12:46 UTC (permalink / raw) To: Mathieu Poirier Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland Mathieu Poirier <mathieu.poirier@linaro.org> writes: > On 27 January 2017 at 05:12, Alexander Shishkin > <alexander.shishkin@linux.intel.com> wrote: >> But "range" is not an action, it's a type of a filter. It determines the >> condition that triggers an action. An action, however, is what we do >> when the condition comes true. > > Then filter->action could be renamed 'type'. No. Again, *action* is what we *do*. *Type* is *how* we detect that something needs to be done. > In the end filters on PT > are range filters, the same way they are on CS. But changing the No. The CS driver supports both single address and address range filters at least acconding to my reading of the code. Now that I look more at it, I see that it also gets the range filters wrong: it disregards filter->filter for range filters, assuming that since it's a range, it means that the user wants to trace what's in the range (filter->filter == 1), but it may also mean "stop if you end up in this range" (filter->filter == 0). 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. In the case of CS, I think that a -EOPNOTSUPP is also appropriate for the type==range&&action==stop combination. Regards, -- Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-02-01 12:46 ` Alexander Shishkin @ 2017-02-01 21:33 ` Mathieu Poirier 2017-02-01 22:15 ` Mathieu Poirier 2017-02-02 16:22 ` Alexander Shishkin 0 siblings, 2 replies; 28+ messages in thread From: Mathieu Poirier @ 2017-02-01 21:33 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland ) On 1 February 2017 at 05:46, Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > Mathieu Poirier <mathieu.poirier@linaro.org> writes: > >> On 27 January 2017 at 05:12, Alexander Shishkin >> <alexander.shishkin@linux.intel.com> wrote: >>> But "range" is not an action, it's a type of a filter. It determines the >>> condition that triggers an action. An action, however, is what we do >>> when the condition comes true. >> >> Then filter->action could be renamed 'type'. > > No. Again, *action* is what we *do*. *Type* is *how* we detect that > something needs to be done. 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 + * @action: 1: start filtering 0: stop filtering will avoid any confusion. > >> In the end filters on PT >> are range filters, the same way they are on CS. But changing the > > No. The CS driver supports both single address and address range > filters at least acconding to my reading of the code. Now that I look > more at it, I see that it also gets the range filters wrong: it > disregards filter->filter for range filters, assuming that since it's a > range, it means that the user wants to trace what's in the range > (filter->filter == 1), but it may also mean "stop if you end up in this > range" (filter->filter == 0). Exactly. The code does the right thing based on my interpretation of the comment found in the code: * @range: 1: range, 0: address * @filter: 1: filter/start, 0: stop That is @range to determine if we are using a range or an address filter and @filter to specify what kind of address filter to use (start or stop). Ignoring range filters when ->filter == 0 was done on purpose as I simply couldn't see how to fit it in. > 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. To move forward please keep the current functionality on the CS side, i.e return -EINVAL when a size of zero is used with a range filter. Once it is queued I'll send a set of patches to support the exclusion of address ranges. > In the case of CS, I think that a -EOPNOTSUPP is also appropriate for > the type==range&&action==stop combination. That will also be part of said patches. Thanks, Mathieu > > Regards, > -- > Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-02-01 21:33 ` Mathieu Poirier @ 2017-02-01 22:15 ` Mathieu Poirier 2017-02-02 10:42 ` Alexander Shishkin 2017-02-02 16:22 ` Alexander Shishkin 1 sibling, 1 reply; 28+ messages in thread From: Mathieu Poirier @ 2017-02-01 22:15 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland On 1 February 2017 at 14:33, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > ) > > On 1 February 2017 at 05:46, Alexander Shishkin > <alexander.shishkin@linux.intel.com> wrote: >> Mathieu Poirier <mathieu.poirier@linaro.org> writes: >> >>> On 27 January 2017 at 05:12, Alexander Shishkin >>> <alexander.shishkin@linux.intel.com> wrote: >>>> But "range" is not an action, it's a type of a filter. It determines the >>>> condition that triggers an action. An action, however, is what we do >>>> when the condition comes true. >>> >>> Then filter->action could be renamed 'type'. >> >> No. Again, *action* is what we *do*. *Type* is *how* we detect that >> something needs to be done. > > 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 > > + * @action: 1: start filtering 0: stop filtering > > will avoid any confusion. > >> >>> In the end filters on PT >>> are range filters, the same way they are on CS. But changing the >> >> No. The CS driver supports both single address and address range >> filters at least acconding to my reading of the code. Now that I look >> more at it, I see that it also gets the range filters wrong: it >> disregards filter->filter for range filters, assuming that since it's a >> range, it means that the user wants to trace what's in the range >> (filter->filter == 1), but it may also mean "stop if you end up in this >> range" (filter->filter == 0). > > Exactly. The code does the right thing based on my interpretation of > the comment found in the code: > > * @range: 1: range, 0: address > * @filter: 1: filter/start, 0: stop > > That is @range to determine if we are using a range or an address > filter and @filter to specify what kind of address filter to use > (start or stop). Ignoring range filters when ->filter == 0 was done > on purpose as I simply couldn't see how to fit it in. > >> 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. > > To move forward please keep the current functionality on the CS side, > i.e return -EINVAL when a size of zero is used with a range filter. > Once it is queued I'll send a set of patches to support the exclusion > of address ranges. > >> In the case of CS, I think that a -EOPNOTSUPP is also appropriate for >> the type==range&&action==stop combination. > > That will also be part of said patches. > > Thanks, > Mathieu Furthermore... static const match_table_t if_tokens = { { IF_ACT_FILTER, "filter" }, { IF_ACT_START, "start" }, { IF_ACT_STOP, "stop" }, { IF_SRC_FILE, "%u/%u@%s" }, { IF_SRC_KERNEL, "%u/%u" }, { IF_SRC_FILEADDR, "%u@%s" }, { IF_SRC_KERNELADDR, "%u" }, { IF_ACT_NONE, NULL }, }; Do we have two different syntax to specify the same behaviour? For example we have: --filter 'start 0x80082570/0x644' and --filter 'filter 0x80082570/0x644' Both will end up with filter->filter == 1 and filter->range == 1. The same will be true for: --filter 'start 0x80082570' and --filter 'filter 0x80082570' ends up with filter->filter == 1 and filter->range == 0. > >> >> Regards, >> -- >> Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-02-01 22:15 ` Mathieu Poirier @ 2017-02-02 10:42 ` Alexander Shishkin 2017-02-02 17:36 ` Mathieu Poirier 0 siblings, 1 reply; 28+ messages in thread From: Alexander Shishkin @ 2017-02-02 10:42 UTC (permalink / raw) To: Mathieu Poirier Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland Mathieu Poirier <mathieu.poirier@linaro.org> writes: > Do we have two different syntax to specify the same behaviour? > > For example we have: > > --filter 'start 0x80082570/0x644' > > and > > --filter 'filter 0x80082570/0x644' > > Both will end up with filter->filter == 1 and filter->range == 1. This is another reason why enum action is needed. The difference between 'start' and 'filter' is that the former means "start tracing when you enter this region until something else stops it"; the latter means "trace only inside this region" (that is, start tracing when you branch inside this region and stop when you branch outside). They cannot be treated interchangeably as I originally though. PT supports 'filter', CS supports 'start', if I remember right. So we should make sure to -EOPNOTSUPP things that we don't actually support. Regards, -- Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-02-02 10:42 ` Alexander Shishkin @ 2017-02-02 17:36 ` Mathieu Poirier 0 siblings, 0 replies; 28+ messages in thread From: Mathieu Poirier @ 2017-02-02 17:36 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland On 2 February 2017 at 03:42, Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > Mathieu Poirier <mathieu.poirier@linaro.org> writes: > >> Do we have two different syntax to specify the same behaviour? >> >> For example we have: >> >> --filter 'start 0x80082570/0x644' >> >> and >> >> --filter 'filter 0x80082570/0x644' >> >> Both will end up with filter->filter == 1 and filter->range == 1. > > This is another reason why enum action is needed. The difference between > 'start' and 'filter' is that the former means "start tracing when you > enter this region until something else stops it"; And what is the "something else here"? > the latter means > "trace only inside this region" (that is, start tracing when you branch > inside this region and stop when you branch outside). That is indeed how range filters work on CS. > They cannot be > treated interchangeably as I originally though. PT supports 'filter', CS > supports 'start', if I remember right. So we should make sure to > -EOPNOTSUPP things that we don't actually support. I already published slides at 2 conferences that uses "filter" for range filters. On CS I will have to continue using "filter" and "start" (when specified with a size element) as one and the same. > > Regards, > -- > Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-02-01 21:33 ` Mathieu Poirier 2017-02-01 22:15 ` Mathieu Poirier @ 2017-02-02 16:22 ` Alexander Shishkin 2017-02-07 17:50 ` Mathieu Poirier 1 sibling, 1 reply; 28+ messages in thread From: Alexander Shishkin @ 2017-02-02 16:22 UTC (permalink / raw) To: Mathieu Poirier Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland Mathieu Poirier <mathieu.poirier@linaro.org> 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. 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. >From 0a549c016591b470fc46b79664ec761db1db6666 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 $WRITEME 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 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. + */ + 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; @@ -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; } 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". */ 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2017-02-02 16:22 ` Alexander Shishkin @ 2017-02-07 17:50 ` Mathieu Poirier [not found] ` <20180117123137.3hlmudzu5eogl53n@ukko.fi.intel.com> 0 siblings, 1 reply; 28+ messages in thread From: Mathieu Poirier @ 2017-02-07 17:50 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland On 2 February 2017 at 09:22, Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > Mathieu Poirier <mathieu.poirier@linaro.org> 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@linux.intel.com> > 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@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 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <20180117123137.3hlmudzu5eogl53n@ukko.fi.intel.com>]
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure [not found] ` <20180117123137.3hlmudzu5eogl53n@ukko.fi.intel.com> @ 2018-01-18 16:59 ` Mathieu Poirier 2018-01-18 17:06 ` Will Deacon 2018-01-19 18:50 ` Mathieu Poirier 1 sibling, 1 reply; 28+ messages in thread From: Mathieu Poirier @ 2018-01-18 16:59 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland 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! 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@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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2018-01-18 16:59 ` Mathieu Poirier @ 2018-01-18 17:06 ` Will Deacon 2018-01-18 18:19 ` Mathieu Poirier 0 siblings, 1 reply; 28+ messages in thread From: Will Deacon @ 2018-01-18 17:06 UTC (permalink / raw) To: Mathieu Poirier Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Mark Rutland On Thu, Jan 18, 2018 at 09:59:26AM -0700, Mathieu Poirier wrote: > 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! > > Oh boy! That's a whole year ago... Give me some time to wrap my > brain around it again. Do we need anything for SPE, or is this only applicable to certain types of tracing PMUs? Will ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure 2018-01-18 17:06 ` Will Deacon @ 2018-01-18 18:19 ` Mathieu Poirier 0 siblings, 0 replies; 28+ messages in thread From: Mathieu Poirier @ 2018-01-18 18:19 UTC (permalink / raw) To: Will Deacon Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Mark Rutland On 18 January 2018 at 10:06, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Jan 18, 2018 at 09:59:26AM -0700, Mathieu Poirier wrote: >> 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! >> >> Oh boy! That's a whole year ago... Give me some time to wrap my >> brain around it again. > > Do we need anything for SPE, or is this only applicable to certain types of > tracing PMUs? As far as I can tell spe_pmu->pmu->nr_addr_filters isn't set anywhere. A such SPE isn't concerned here. Mathieu > > Will ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure [not found] ` <20180117123137.3hlmudzu5eogl53n@ukko.fi.intel.com> 2018-01-18 16:59 ` Mathieu Poirier @ 2018-01-19 18:50 ` Mathieu Poirier 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Poirier @ 2018-01-19 18:50 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/3] perf: Do error out on a kernel filter on an exclude_filter event 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 9:40 ` 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 2 siblings, 2 replies; 28+ messages in thread From: Alexander Shishkin @ 2017-01-26 9:40 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland, Alexander Shishkin, Mathieu Poirier It is currently possible to configure a kernel address filter for a event that excludes kernel from its traces (attr.exclude_kernel==1). While in reality this doesn't make sense, the SET_FILTER ioctl() should return a error in such case, currently it does not. Furthermore, it will still silently discard the filter and any potentially valid filters that came with it. This patch makes the SET_FILTER ioctl() error out in such cases. Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> --- kernel/events/core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kernel/events/core.c b/kernel/events/core.c index b422b5feee..36770a13ef 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -8261,6 +8261,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, * attribute. */ if (state == IF_STATE_END) { + ret = -EINVAL; if (kernel && event->attr.exclude_kernel) goto fail; -- 2.11.0 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/3] perf: Do error out on a kernel filter on an exclude_filter event 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 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Poirier @ 2017-01-26 18:32 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland On Thu, Jan 26, 2017 at 11:40:56AM +0200, Alexander Shishkin wrote: > It is currently possible to configure a kernel address filter for a > event that excludes kernel from its traces (attr.exclude_kernel==1). > > While in reality this doesn't make sense, the SET_FILTER ioctl() should > return a error in such case, currently it does not. Furthermore, it > will still silently discard the filter and any potentially valid filters > that came with it. > > This patch makes the SET_FILTER ioctl() error out in such cases. > > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > kernel/events/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index b422b5feee..36770a13ef 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -8261,6 +8261,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, > * attribute. > */ > if (state == IF_STATE_END) { > + ret = -EINVAL; > if (kernel && event->attr.exclude_kernel) > goto fail; Yes, I stumbled on that when implementing filters on CS and could swear I had a patch for it. It obviously never got sent out. Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [tip:perf/core] perf/core: Do error out on a kernel filter on an exclude_filter event 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-bot for Alexander Shishkin 1 sibling, 0 replies; 28+ messages in thread From: tip-bot for Alexander Shishkin @ 2017-02-10 8:33 UTC (permalink / raw) To: linux-tip-commits Cc: hpa, will.deacon, tglx, vincent.weaver, mingo, peterz, jolsa, alexander.shishkin, eranian, mark.rutland, acme, mathieu.poirier, torvalds, linux-kernel, acme Commit-ID: 9ccbfbb157a38921702402281ca7be530b4c3669 Gitweb: http://git.kernel.org/tip/9ccbfbb157a38921702402281ca7be530b4c3669 Author: Alexander Shishkin <alexander.shishkin@linux.intel.com> AuthorDate: Thu, 26 Jan 2017 11:40:56 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Fri, 10 Feb 2017 09:08:09 +0100 perf/core: Do error out on a kernel filter on an exclude_filter event It is currently possible to configure a kernel address filter for a event that excludes kernel from its traces (attr.exclude_kernel==1). While in reality this doesn't make sense, the SET_FILTER ioctl() should return a error in such case, currently it does not. Furthermore, it will still silently discard the filter and any potentially valid filters that came with it. This patch makes the SET_FILTER ioctl() error out in such cases. Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Arnaldo Carvalho de Melo <acme@infradead.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Stephane Eranian <eranian@google.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Vince Weaver <vincent.weaver@maine.edu> Cc: Will Deacon <will.deacon@arm.com> Cc: vince@deater.net Link: http://lkml.kernel.org/r/20170126094057.13805-3-alexander.shishkin@linux.intel.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- kernel/events/core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kernel/events/core.c b/kernel/events/core.c index 88676ff..1730995 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -8260,6 +8260,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, * attribute. */ if (state == IF_STATE_END) { + ret = -EINVAL; if (kernel && event->attr.exclude_kernel) goto fail; ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/3] perf: Allow kernel filters on cpu events 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 9:40 ` [PATCH 2/3] perf: Do error out on a kernel filter on an exclude_filter event Alexander Shishkin @ 2017-01-26 9:40 ` Alexander Shishkin 2017-01-26 21:38 ` Mathieu Poirier ` (2 more replies) 2 siblings, 3 replies; 28+ messages in thread From: Alexander Shishkin @ 2017-01-26 9:40 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland, Alexander Shishkin, Mathieu Poirier While supporting file-based address filters for cpu events requires some extra context switch handling, kernel address filters are easy, since the kernel mapping is preserved across address spaces. It is also useful as it permits tracing scheduling paths of the kernel. This patch allows setting up kernel filters for cpu events. Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> --- include/linux/perf_event.h | 2 ++ kernel/events/core.c | 42 ++++++++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index fcb37c81ca..f4ea0600b2 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -486,6 +486,7 @@ struct perf_addr_filter { * @list: list of filters for this event * @lock: spinlock that serializes accesses to the @list and event's * (and its children's) filter generations. + * @nr_file_filters: number of file-based filters * * A child event will use parent's @list (and therefore @lock), so they are * bundled together; see perf_event_addr_filters(). @@ -493,6 +494,7 @@ struct perf_addr_filter { struct perf_addr_filters_head { struct list_head list; raw_spinlock_t lock; + unsigned int nr_file_filters; }; /** diff --git a/kernel/events/core.c b/kernel/events/core.c index 36770a13ef..2eeb8fec2f 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -8093,6 +8093,9 @@ static void perf_event_addr_filters_apply(struct perf_event *event) if (task == TASK_TOMBSTONE) return; + if (!ifh->nr_file_filters) + return; + mm = get_task_mm(event->ctx->task); if (!mm) goto restart; @@ -8269,6 +8272,18 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, if (!filename) goto fail; + /* + * For now, we only support file-based filters + * in per-task events; doing so for CPU-wide + * events requires additional context switching + * trickery, since same object code will be + * mapped at different virtual addresses in + * different processes. + */ + ret = -EOPNOTSUPP; + if (!event->ctx->task) + goto fail_free_name; + /* look up the path and grab its inode */ ret = kern_path(filename, LOOKUP_FOLLOW, &path); if (ret) @@ -8284,6 +8299,8 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, !S_ISREG(filter->inode->i_mode)) /* free_filters_list() will iput() */ goto fail; + + event->addr_filters.nr_file_filters++; } /* ready to consume more filters */ @@ -8323,24 +8340,13 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str) if (WARN_ON_ONCE(event->parent)) return -EINVAL; - /* - * For now, we only support filtering in per-task events; doing so - * for CPU-wide events requires additional context switching trickery, - * since same object code will be mapped at different virtual - * addresses in different processes. - */ - if (!event->ctx->task) - return -EOPNOTSUPP; - ret = perf_event_parse_addr_filter(event, filter_str, &filters); if (ret) - return ret; + goto fail_clear_files; ret = event->pmu->addr_filters_validate(&filters); - if (ret) { - free_filters_list(&filters); - return ret; - } + if (ret) + goto fail_free_filters; /* remove existing filters, if any */ perf_addr_filters_splice(event, &filters); @@ -8349,6 +8355,14 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str) perf_event_for_each_child(event, perf_event_addr_filters_apply); return ret; + +fail_free_filters: + free_filters_list(&filters); + +fail_clear_files: + event->addr_filters.nr_file_filters = 0; + + return ret; } static int perf_event_set_filter(struct perf_event *event, void __user *arg) -- 2.11.0 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] perf: Allow kernel filters on cpu events 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-02-10 8:07 ` Ingo Molnar 2017-02-10 8:34 ` [tip:perf/core] perf/core: Allow kernel filters on CPU events tip-bot for Alexander Shishkin 2 siblings, 1 reply; 28+ messages in thread From: Mathieu Poirier @ 2017-01-26 21:38 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland On Thu, Jan 26, 2017 at 11:40:57AM +0200, Alexander Shishkin wrote: > While supporting file-based address filters for cpu events requires some > extra context switch handling, kernel address filters are easy, since the > kernel mapping is preserved across address spaces. It is also useful as > it permits tracing scheduling paths of the kernel. > > This patch allows setting up kernel filters for cpu events. > > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > include/linux/perf_event.h | 2 ++ > kernel/events/core.c | 42 ++++++++++++++++++++++++++++-------------- > 2 files changed, 30 insertions(+), 14 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index fcb37c81ca..f4ea0600b2 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -486,6 +486,7 @@ struct perf_addr_filter { > * @list: list of filters for this event > * @lock: spinlock that serializes accesses to the @list and event's > * (and its children's) filter generations. > + * @nr_file_filters: number of file-based filters > * > * A child event will use parent's @list (and therefore @lock), so they are > * bundled together; see perf_event_addr_filters(). > @@ -493,6 +494,7 @@ struct perf_addr_filter { > struct perf_addr_filters_head { > struct list_head list; > raw_spinlock_t lock; > + unsigned int nr_file_filters; > }; > > /** > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 36770a13ef..2eeb8fec2f 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -8093,6 +8093,9 @@ static void perf_event_addr_filters_apply(struct perf_event *event) > if (task == TASK_TOMBSTONE) > return; > > + if (!ifh->nr_file_filters) > + return; Is this mandatory or an optimisation to avoid circling through a list of filters that don't included user space files? > + > mm = get_task_mm(event->ctx->task); > if (!mm) > goto restart; > @@ -8269,6 +8272,18 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, > if (!filename) > goto fail; > > + /* > + * For now, we only support file-based filters > + * in per-task events; doing so for CPU-wide > + * events requires additional context switching > + * trickery, since same object code will be > + * mapped at different virtual addresses in > + * different processes. > + */ > + ret = -EOPNOTSUPP; > + if (!event->ctx->task) > + goto fail_free_name; > + > /* look up the path and grab its inode */ > ret = kern_path(filename, LOOKUP_FOLLOW, &path); > if (ret) > @@ -8284,6 +8299,8 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, > !S_ISREG(filter->inode->i_mode)) > /* free_filters_list() will iput() */ > goto fail; > + > + event->addr_filters.nr_file_filters++; > } > > /* ready to consume more filters */ > @@ -8323,24 +8340,13 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str) > if (WARN_ON_ONCE(event->parent)) > return -EINVAL; > > - /* > - * For now, we only support filtering in per-task events; doing so > - * for CPU-wide events requires additional context switching trickery, > - * since same object code will be mapped at different virtual > - * addresses in different processes. > - */ > - if (!event->ctx->task) > - return -EOPNOTSUPP; > - > ret = perf_event_parse_addr_filter(event, filter_str, &filters); > if (ret) > - return ret; > + goto fail_clear_files; > > ret = event->pmu->addr_filters_validate(&filters); > - if (ret) { > - free_filters_list(&filters); > - return ret; > - } > + if (ret) > + goto fail_free_filters; > > /* remove existing filters, if any */ > perf_addr_filters_splice(event, &filters); > @@ -8349,6 +8355,14 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str) > perf_event_for_each_child(event, perf_event_addr_filters_apply); > > return ret; > + > +fail_free_filters: > + free_filters_list(&filters); > + > +fail_clear_files: > + event->addr_filters.nr_file_filters = 0; > + > + return ret; > } > > static int perf_event_set_filter(struct perf_event *event, void __user *arg) > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] perf: Allow kernel filters on cpu events 2017-01-26 21:38 ` Mathieu Poirier @ 2017-01-27 12:31 ` Alexander Shishkin 2017-01-27 17:38 ` Mathieu Poirier 0 siblings, 1 reply; 28+ messages in thread From: Alexander Shishkin @ 2017-01-27 12:31 UTC (permalink / raw) To: Mathieu Poirier Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland Mathieu Poirier <mathieu.poirier@linaro.org> writes: > On Thu, Jan 26, 2017 at 11:40:57AM +0200, Alexander Shishkin wrote: >> + if (!ifh->nr_file_filters) >> + return; > > Is this mandatory or an optimisation to avoid circling through a list of filters > that don't included user space files? It's both. It stems from the fact that the remainder of this function relies on ctx::task not being NULL, which is not the case with cpu contexts and now that we've enabled address filters for such contexts, it's a problem. So checking for !task would have done the trick here, but this way we'll also avoid going down this path for task contexts in the absence of file-based filters. In particular, grabbing the mmap semaphore and filters spinlock we can do without. Regards, -- Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] perf: Allow kernel filters on cpu events 2017-01-27 12:31 ` Alexander Shishkin @ 2017-01-27 17:38 ` Mathieu Poirier 0 siblings, 0 replies; 28+ messages in thread From: Mathieu Poirier @ 2017-01-27 17:38 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Vince Weaver, Stephane Eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland On 27 January 2017 at 05:31, Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > Mathieu Poirier <mathieu.poirier@linaro.org> writes: > >> On Thu, Jan 26, 2017 at 11:40:57AM +0200, Alexander Shishkin wrote: >>> + if (!ifh->nr_file_filters) >>> + return; >> >> Is this mandatory or an optimisation to avoid circling through a list of filters >> that don't included user space files? > > It's both. It stems from the fact that the remainder of this function > relies on ctx::task not being NULL, which is not the case with cpu > contexts and now that we've enabled address filters for such contexts, > it's a problem. So checking for !task would have done the trick here, > but this way we'll also avoid going down this path for task contexts > in the absence of file-based filters. In particular, grabbing the mmap > semaphore and filters spinlock we can do without. Yes, I see it now. Do you have bigger plans for ->nr_file_filters? If the purpose is only to indicate the presence of user space files then it should be a 'bool' type (and probably changed to ->file_filters). Thanks, Mathieu > > Regards, > -- > Alex ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] perf: Allow kernel filters on cpu events 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-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 2 siblings, 1 reply; 28+ messages in thread From: Ingo Molnar @ 2017-02-10 8:07 UTC (permalink / raw) To: Alexander Shishkin Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland, Mathieu Poirier * Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > While supporting file-based address filters for cpu events requires some > extra context switch handling, kernel address filters are easy, since the > kernel mapping is preserved across address spaces. It is also useful as > it permits tracing scheduling paths of the kernel. > > This patch allows setting up kernel filters for cpu events. > > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > include/linux/perf_event.h | 2 ++ > kernel/events/core.c | 42 ++++++++++++++++++++++++++++-------------- > 2 files changed, 30 insertions(+), 14 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index fcb37c81ca..f4ea0600b2 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -486,6 +486,7 @@ struct perf_addr_filter { > * @list: list of filters for this event > * @lock: spinlock that serializes accesses to the @list and event's > * (and its children's) filter generations. > + * @nr_file_filters: number of file-based filters I've applied the patch, but please for heaven's sake, when you add such a long, long argument name, at minimum re-tabulate the documentation section to look good again ... Thanks, Ingo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] perf: Allow kernel filters on cpu events 2017-02-10 8:07 ` Ingo Molnar @ 2017-02-14 12:59 ` Alexander Shishkin 0 siblings, 0 replies; 28+ messages in thread From: Alexander Shishkin @ 2017-02-14 12:59 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, vince, eranian, Arnaldo Carvalho de Melo, Will Deacon, Mark Rutland, Mathieu Poirier Ingo Molnar <mingo@kernel.org> writes: > * Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > >> While supporting file-based address filters for cpu events requires some >> extra context switch handling, kernel address filters are easy, since the >> kernel mapping is preserved across address spaces. It is also useful as >> it permits tracing scheduling paths of the kernel. >> >> This patch allows setting up kernel filters for cpu events. >> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> --- >> include/linux/perf_event.h | 2 ++ >> kernel/events/core.c | 42 ++++++++++++++++++++++++++++-------------- >> 2 files changed, 30 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> index fcb37c81ca..f4ea0600b2 100644 >> --- a/include/linux/perf_event.h >> +++ b/include/linux/perf_event.h >> @@ -486,6 +486,7 @@ struct perf_addr_filter { >> * @list: list of filters for this event >> * @lock: spinlock that serializes accesses to the @list and event's >> * (and its children's) filter generations. >> + * @nr_file_filters: number of file-based filters > > I've applied the patch, but please for heaven's sake, when you add such a long, > long argument name, at minimum re-tabulate the documentation section to look good > again ... Will do. >From 8b993fa566acb0cd333ce5e433f1e6858b181f58 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin <alexander.shishkin@linux.intel.com> Date: Tue, 14 Feb 2017 14:49:41 +0200 Subject: [PATCH] perf: Fix alignment in a kerneldoc comment Commit 6ce77bfd6c ("perf/core: Allow kernel filters on CPU events") added "nr_file_filters" to the filters structure and its kerneldoc description, which now requires that other fields' descriptions be padded to the next tabstop to not look ugly. Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> --- include/linux/perf_event.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 258ebdb533..8f347b84cf 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -503,9 +503,9 @@ struct perf_addr_filter { /** * struct perf_addr_filters_head - container for address range filters - * @list: list of filters for this event - * @lock: spinlock that serializes accesses to the @list and event's - * (and its children's) filter generations. + * @list: list of filters for this event + * @lock: spinlock that serializes accesses to the @list and + * event's (and its children's) filter generations. * @nr_file_filters: number of file-based filters * * A child event will use parent's @list (and therefore @lock), so they are -- 2.11.0 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [tip:perf/core] perf/core: Allow kernel filters on CPU events 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-02-10 8:07 ` Ingo Molnar @ 2017-02-10 8:34 ` tip-bot for Alexander Shishkin 2 siblings, 0 replies; 28+ messages in thread From: tip-bot for Alexander Shishkin @ 2017-02-10 8:34 UTC (permalink / raw) To: linux-tip-commits Cc: jolsa, eranian, vincent.weaver, mark.rutland, linux-kernel, acme, will.deacon, mingo, peterz, tglx, acme, hpa, mathieu.poirier, alexander.shishkin, torvalds Commit-ID: 6ce77bfd6cedbff61eabf8837dc0901bb671cc86 Gitweb: http://git.kernel.org/tip/6ce77bfd6cedbff61eabf8837dc0901bb671cc86 Author: Alexander Shishkin <alexander.shishkin@linux.intel.com> AuthorDate: Thu, 26 Jan 2017 11:40:57 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Fri, 10 Feb 2017 09:08:09 +0100 perf/core: Allow kernel filters on CPU events While supporting file-based address filters for CPU events requires some extra context switch handling, kernel address filters are easy, since the kernel mapping is preserved across address spaces. It is also useful as it permits tracing scheduling paths of the kernel. This patch allows setting up kernel filters for CPU events. Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Arnaldo Carvalho de Melo <acme@infradead.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Stephane Eranian <eranian@google.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Vince Weaver <vincent.weaver@maine.edu> Cc: Will Deacon <will.deacon@arm.com> Cc: vince@deater.net Link: http://lkml.kernel.org/r/20170126094057.13805-4-alexander.shishkin@linux.intel.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- include/linux/perf_event.h | 2 ++ kernel/events/core.c | 42 ++++++++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 5c58e93..000fdb2 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -482,6 +482,7 @@ struct perf_addr_filter { * @list: list of filters for this event * @lock: spinlock that serializes accesses to the @list and event's * (and its children's) filter generations. + * @nr_file_filters: number of file-based filters * * A child event will use parent's @list (and therefore @lock), so they are * bundled together; see perf_event_addr_filters(). @@ -489,6 +490,7 @@ struct perf_addr_filter { struct perf_addr_filters_head { struct list_head list; raw_spinlock_t lock; + unsigned int nr_file_filters; }; /** diff --git a/kernel/events/core.c b/kernel/events/core.c index 1730995..a866424 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -8090,6 +8090,9 @@ static void perf_event_addr_filters_apply(struct perf_event *event) if (task == TASK_TOMBSTONE) return; + if (!ifh->nr_file_filters) + return; + mm = get_task_mm(event->ctx->task); if (!mm) goto restart; @@ -8268,6 +8271,18 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, if (!filename) goto fail; + /* + * For now, we only support file-based filters + * in per-task events; doing so for CPU-wide + * events requires additional context switching + * trickery, since same object code will be + * mapped at different virtual addresses in + * different processes. + */ + ret = -EOPNOTSUPP; + if (!event->ctx->task) + goto fail_free_name; + /* look up the path and grab its inode */ ret = kern_path(filename, LOOKUP_FOLLOW, &path); if (ret) @@ -8283,6 +8298,8 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr, !S_ISREG(filter->inode->i_mode)) /* free_filters_list() will iput() */ goto fail; + + event->addr_filters.nr_file_filters++; } /* ready to consume more filters */ @@ -8322,24 +8339,13 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str) if (WARN_ON_ONCE(event->parent)) return -EINVAL; - /* - * For now, we only support filtering in per-task events; doing so - * for CPU-wide events requires additional context switching trickery, - * since same object code will be mapped at different virtual - * addresses in different processes. - */ - if (!event->ctx->task) - return -EOPNOTSUPP; - ret = perf_event_parse_addr_filter(event, filter_str, &filters); if (ret) - return ret; + goto fail_clear_files; ret = event->pmu->addr_filters_validate(&filters); - if (ret) { - free_filters_list(&filters); - return ret; - } + if (ret) + goto fail_free_filters; /* remove existing filters, if any */ perf_addr_filters_splice(event, &filters); @@ -8348,6 +8354,14 @@ perf_event_set_addr_filter(struct perf_event *event, char *filter_str) perf_event_for_each_child(event, perf_event_addr_filters_apply); return ret; + +fail_free_filters: + free_filters_list(&filters); + +fail_clear_files: + event->addr_filters.nr_file_filters = 0; + + return ret; } static int perf_event_set_filter(struct perf_event *event, void __user *arg) ^ permalink raw reply related [flat|nested] 28+ messages in thread
end of thread, other threads:[~2018-01-19 18:50 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).