From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752264AbdBAVds (ORCPT ); Wed, 1 Feb 2017 16:33:48 -0500 Received: from mail-yw0-f170.google.com ([209.85.161.170]:32970 "EHLO mail-yw0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751287AbdBAVdr (ORCPT ); Wed, 1 Feb 2017 16:33:47 -0500 MIME-Version: 1.0 In-Reply-To: <87wpda9iyv.fsf@ashishki-desk.ger.corp.intel.com> References: <20170126094057.13805-1-alexander.shishkin@linux.intel.com> <20170126094057.13805-2-alexander.shishkin@linux.intel.com> <20170126182645.GA1991@linaro.org> <87h94kbt29.fsf@ashishki-desk.ger.corp.intel.com> <87wpda9iyv.fsf@ashishki-desk.ger.corp.intel.com> From: Mathieu Poirier Date: Wed, 1 Feb 2017 14:33:46 -0700 Message-ID: Subject: Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure To: Alexander Shishkin Cc: Peter Zijlstra , Ingo Molnar , "linux-kernel@vger.kernel.org" , Vince Weaver , Stephane Eranian , Arnaldo Carvalho de Melo , Will Deacon , Mark Rutland Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ) On 1 February 2017 at 05:46, Alexander Shishkin wrote: > Mathieu Poirier writes: > >> On 27 January 2017 at 05:12, Alexander Shishkin >> 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