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

[Mathieu Poirier]

On 1 February 2017 at 14:33, Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> wrote:
> )
>
> On 1 February 2017 at 05:46, Alexander Shishkin
> <alexander.shishkin@xxxxxxxxxxxxxxx> wrote:
>> Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> writes:
>>
>>> On 27 January 2017 at 05:12, Alexander Shishkin
>>> <alexander.shishkin@xxxxxxxxxxxxxxx> 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