Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1163784AbdD0RiN (ORCPT ); Thu, 27 Apr 2017 13:38:13 -0400 Received: from foss.arm.com ([217.140.101.70]:40250 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751355AbdD0Rh7 (ORCPT ); Thu, 27 Apr 2017 13:37:59 -0400 Date: Thu, 27 Apr 2017 18:37:59 +0100 From: Will Deacon To: Jayachandran C Cc: "Pinski, Andrew" , "Jayachandran C." , Ganapatrao Kulkarni , Mark Rutland , Catalin Marinas , "linux-kernel@vger.kernel.org" , "acme@kernel.org" , "alexander.shishkin@linux.intel.com" , "peterz@infradead.org" , Ingo Molnar , "Nair, Jayachandran" , "Kulkarni, Ganapatrao" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP Message-ID: <20170427173758.GN1890@arm.com> References: <1492623846-29335-1-git-send-email-ganapatrao.kulkarni@cavium.com> <20170420084928.GC31436@leverpostej> <20170424154530.GO12323@arm.com> <20170425165259.GS24484@arm.com> <20170426101021.GF21744@arm.com> <20170426134141.GA6417@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170426134141.GA6417@localhost> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3773 Lines: 70 On Wed, Apr 26, 2017 at 01:41:42PM +0000, Jayachandran C wrote: > On Wed, Apr 26, 2017 at 11:10:21AM +0100, Will Deacon wrote: > > On Wed, Apr 26, 2017 at 07:22:46AM +0000, Pinski, Andrew wrote: > > > On 4/25/2017 11:53 PM, Jayachandran C. wrote: > > > > On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon wrote: > > > >> On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote: > > > >>> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon wrote: > > > >>>> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote: > > > >>>>> OK, if you are ok with sysfs part, i can send next version with that > > > >>>>> change only?. > > > >>>> I think the sysfs part is still a little dodgy, since you still expose the > > > >>>> "exclude_hv" file with a value of 0 when not running at EL2, which would > > > >>>> imply that exclude_hv is forced to zero. I don't think that's correct. > > > >>> okay, i can make exclude_hv visible only when kernel booted in EL2. > > > >>> is it ok to have empty directory "attr" when kernel booted to EL1? > > > >>> attr can be place holder for any other miscellaneous attributes, that > > > >>> can be added in future. > > > >> Sounds good to me, although I'll seek comment from the other perf folks > > > >> before merging anything with ABI implications. > > > > Do you really think this is the solution given: > > > > - this is an arm64 specific sysfs interface that is tied to the perf API > > > > That's why I want feedback from others. The intention would be that this can > > be used by other PMUs as well, since it's not uncommon that parts of the > > sizeable perf_event_attr structure are not used by a given PMU. > > > > > > - the perf API documentation has to be updated for this > > > > So? If having to update documentation means we shouldn't change the kernel, > > then we may as well all find new jobs. > > > > > > - All the applications that use the perf API have to be modified to > > > > check this sysfs interface > > > > - If the application fails to do so, a very narrow corner case > > > > (exclude_hv != exclude_kernel and VHE enabled) fails. > > > > See below, but apparently people care about it. > > > > > > Any application that really cares can already do see if exclude_hv != > > > > exclude_kernel case works by calling perf_open_event() with those > > > > options and checking the return value. > > > > That's a good point: there is *something* userspace can do, although that > > would be arm64-specific and doesn't really help with the state-space > > explosion you get with combinations of invalid/unused perf_event_attr > > fields. > > > > > An example of an application which needs to changed is HHVM. Currently > > > it sets exclude_hv to true but exclude_kernel to false as it does not > > > care about the hypervisor associated perf events associated with the > > > code, only the kernel and userspace associated evnts. > > > Yes we could submit a patch to use the sysfs interface to check but it > > > would look funny and the facebook folks might reject the patch as it is > > > ARM64 specific in generic code. Note this is how all of this discussion > > > started was HHVM's call to perf_open_event was failing. > > > > Hmm, if you're saying that HHVM won't be changed to use the sysfs stuff, > > then why are we bothering? > > > > Not sure where this leaves us. > > If my understanding is correct, the sysfs suggestion above is going to > add API complexity without solving the issue. Ignoring the exclude_hv if > it cannot be honored would be a better solution. Better for HHVM, sure, but I don't think it's better in general. It means that we silently do the opposite of what the user has requested in some configurations. Will