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 <[email protected]> > Cc: Mathieu Poirier <[email protected]> > --- > 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 >
