Messages in this thread | | | From | Mathieu Poirier <> | Date | Wed, 1 Feb 2017 14:33:46 -0700 | Subject | Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure |
| |
)
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
| |