lkml.org 
[lkml]   [2017]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure
    Date
    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
    \
     
     \ /
      Last update: 2017-02-02 17:27    [W:2.743 / U:0.220 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site