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:

quoted

On Wed, Apr 26, 2017 at 07:22:46AM +0000, Pinski, Andrew wrote:

quoted

On 4/25/2017 11:53 PM, Jayachandran C. wrote:

quoted

On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon [off-list ref] wrote:

quoted

On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:

quoted

On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon [off-list ref] wrote:

quoted

On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:

quoted

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.

quoted

quoted

- 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.

quoted

quoted

- 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.

quoted

quoted

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.

quoted

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