From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753930AbdBAWPx (ORCPT ); Wed, 1 Feb 2017 17:15:53 -0500 Received: from mail-yw0-f171.google.com ([209.85.161.171]:35370 "EHLO mail-yw0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753885AbdBAWPv (ORCPT ); Wed, 1 Feb 2017 17:15:51 -0500 MIME-Version: 1.0 In-Reply-To: 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 15:15:50 -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 14:33, Mathieu Poirier wrote: > ) > > 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 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