commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
is returning error for perf syscall with mixed attribute set for exclude_kernel
and exclude_hv. This change is breaking some applications (observed with hhvm)
when ran on VHE enabled platforms.
Adding fix to consider only exclude_kernel attribute when kernel is
running in HYP. Also adding sysfs file to notify the bhehaviour
of attribute exclude_hv.
Signed-off-by: Ganapatrao Kulkarni <[email protected]>
---
Changelog:
V2:
- Changes as per Will Deacon's suggestion.
V1: Initial patch
arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
include/linux/perf/arm_pmu.h | 1 +
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 57ae9d9..b35b94c 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -535,6 +535,25 @@
.attrs = armv8_pmuv3_format_attrs,
};
+static ssize_t armv8_pmuv3_exclude_hv_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", is_kernel_in_hyp_mode() ? 1 : 0);
+}
+
+static struct device_attribute armv8_pmuv3_exclude_hv_attr =
+ __ATTR(exclude_hv, 0444, armv8_pmuv3_exclude_hv_show, NULL);
+
+static struct attribute *armv8_pmuv3_attrs[] = {
+ &armv8_pmuv3_exclude_hv_attr.attr,
+ NULL,
+};
+
+static struct attribute_group armv8_pmuv3_attr_group = {
+ .name = "attr",
+ .attrs = armv8_pmuv3_attrs,
+};
+
/*
* Perf Events' indices
*/
@@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
if (attr->exclude_idle)
return -EPERM;
- if (is_kernel_in_hyp_mode() &&
- attr->exclude_kernel != attr->exclude_hv)
- return -EINVAL;
+ if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
+ config_base |= ARMV8_PMU_INCLUDE_EL2;
if (attr->exclude_user)
config_base |= ARMV8_PMU_EXCLUDE_EL0;
if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
config_base |= ARMV8_PMU_EXCLUDE_EL1;
- if (!attr->exclude_hv)
+ if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
config_base |= ARMV8_PMU_INCLUDE_EL2;
/*
@@ -1008,6 +1026,8 @@ static int armv8_pmuv3_init(struct arm_pmu *cpu_pmu)
&armv8_pmuv3_events_attr_group;
cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
&armv8_pmuv3_format_attr_group;
+ cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_ATTR] =
+ &armv8_pmuv3_attr_group;
return armv8pmu_probe_pmu(cpu_pmu);
}
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 8462da2..a26ffc7 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -81,6 +81,7 @@ enum armpmu_attr_groups {
ARMPMU_ATTR_GROUP_COMMON,
ARMPMU_ATTR_GROUP_EVENTS,
ARMPMU_ATTR_GROUP_FORMATS,
+ ARMPMU_ATTR_GROUP_ATTR,
ARMPMU_NR_ATTR_GROUPS
};
--
1.8.1.4
On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
> is returning error for perf syscall with mixed attribute set for exclude_kernel
> and exclude_hv. This change is breaking some applications (observed with hhvm)
> when ran on VHE enabled platforms.
>
> Adding fix to consider only exclude_kernel attribute when kernel is
> running in HYP. Also adding sysfs file to notify the bhehaviour
> of attribute exclude_hv.
>
> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> ---
>
> Changelog:
>
> V2:
> - Changes as per Will Deacon's suggestion.
>
> V1: Initial patch
>
> arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
> include/linux/perf/arm_pmu.h | 1 +
> 2 files changed, 25 insertions(+), 4 deletions(-)
>
> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>
> if (attr->exclude_idle)
> return -EPERM;
> - if (is_kernel_in_hyp_mode() &&
> - attr->exclude_kernel != attr->exclude_hv)
> - return -EINVAL;
> + if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
> + config_base |= ARMV8_PMU_INCLUDE_EL2;
> if (attr->exclude_user)
> config_base |= ARMV8_PMU_EXCLUDE_EL0;
> if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
> config_base |= ARMV8_PMU_EXCLUDE_EL1;
> - if (!attr->exclude_hv)
> + if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
> config_base |= ARMV8_PMU_INCLUDE_EL2;
This isn't quite what Will suggested.
The idea was that userspace would read sysfs, then use that to determine
the correct exclusion parameters [1,2]. This logic was not expected to
change; it correctly validates whether we can provide what the user
requests.
Thanks,
Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499224.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499493.html
>
> /*
> @@ -1008,6 +1026,8 @@ static int armv8_pmuv3_init(struct arm_pmu *cpu_pmu)
> &armv8_pmuv3_events_attr_group;
> cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
> &armv8_pmuv3_format_attr_group;
> + cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_ATTR] =
> + &armv8_pmuv3_attr_group;
> return armv8pmu_probe_pmu(cpu_pmu);
> }
>
> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index 8462da2..a26ffc7 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -81,6 +81,7 @@ enum armpmu_attr_groups {
> ARMPMU_ATTR_GROUP_COMMON,
> ARMPMU_ATTR_GROUP_EVENTS,
> ARMPMU_ATTR_GROUP_FORMATS,
> + ARMPMU_ATTR_GROUP_ATTR,
> ARMPMU_NR_ATTR_GROUPS
> };
>
> --
> 1.8.1.4
>
On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <[email protected]> wrote:
> On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
>> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
>> is returning error for perf syscall with mixed attribute set for exclude_kernel
>> and exclude_hv. This change is breaking some applications (observed with hhvm)
>> when ran on VHE enabled platforms.
>>
>> Adding fix to consider only exclude_kernel attribute when kernel is
>> running in HYP. Also adding sysfs file to notify the bhehaviour
>> of attribute exclude_hv.
>>
>> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>> ---
>>
>> Changelog:
>>
>> V2:
>> - Changes as per Will Deacon's suggestion.
>>
>> V1: Initial patch
>>
>> arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
>> include/linux/perf/arm_pmu.h | 1 +
>> 2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>>
>> if (attr->exclude_idle)
>> return -EPERM;
>> - if (is_kernel_in_hyp_mode() &&
>> - attr->exclude_kernel != attr->exclude_hv)
>> - return -EINVAL;
>> + if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
>> + config_base |= ARMV8_PMU_INCLUDE_EL2;
>> if (attr->exclude_user)
>> config_base |= ARMV8_PMU_EXCLUDE_EL0;
>> if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
>> config_base |= ARMV8_PMU_EXCLUDE_EL1;
>> - if (!attr->exclude_hv)
>> + if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
>> config_base |= ARMV8_PMU_INCLUDE_EL2;
>
> This isn't quite what Will suggested.
>
> The idea was that userspace would read sysfs, then use that to determine
> the correct exclusion parameters [1,2]. This logic was not expected to
> change; it correctly validates whether we can provide what the user
> requests.
OK, if you are ok with sysfs part, i can send next version with that
change only?.
>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499224.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499493.html
>
>>
>> /*
>> @@ -1008,6 +1026,8 @@ static int armv8_pmuv3_init(struct arm_pmu *cpu_pmu)
>> &armv8_pmuv3_events_attr_group;
>> cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
>> &armv8_pmuv3_format_attr_group;
>> + cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_ATTR] =
>> + &armv8_pmuv3_attr_group;
>> return armv8pmu_probe_pmu(cpu_pmu);
>> }
>>
>> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
>> index 8462da2..a26ffc7 100644
>> --- a/include/linux/perf/arm_pmu.h
>> +++ b/include/linux/perf/arm_pmu.h
>> @@ -81,6 +81,7 @@ enum armpmu_attr_groups {
>> ARMPMU_ATTR_GROUP_COMMON,
>> ARMPMU_ATTR_GROUP_EVENTS,
>> ARMPMU_ATTR_GROUP_FORMATS,
>> + ARMPMU_ATTR_GROUP_ATTR,
>> ARMPMU_NR_ATTR_GROUPS
>> };
>>
>> --
>> 1.8.1.4
>>
On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <[email protected]> wrote:
> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
> >> when ran on VHE enabled platforms.
> >>
> >> Adding fix to consider only exclude_kernel attribute when kernel is
> >> running in HYP. Also adding sysfs file to notify the bhehaviour
> >> of attribute exclude_hv.
> >>
> >> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> >> ---
> >>
> >> Changelog:
> >>
> >> V2:
> >> - Changes as per Will Deacon's suggestion.
> >>
> >> V1: Initial patch
> >>
> >> arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
> >> include/linux/perf/arm_pmu.h | 1 +
> >> 2 files changed, 25 insertions(+), 4 deletions(-)
> >>
> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> >>
> >> if (attr->exclude_idle)
> >> return -EPERM;
> >> - if (is_kernel_in_hyp_mode() &&
> >> - attr->exclude_kernel != attr->exclude_hv)
> >> - return -EINVAL;
> >> + if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
> >> + config_base |= ARMV8_PMU_INCLUDE_EL2;
> >> if (attr->exclude_user)
> >> config_base |= ARMV8_PMU_EXCLUDE_EL0;
> >> if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
> >> config_base |= ARMV8_PMU_EXCLUDE_EL1;
> >> - if (!attr->exclude_hv)
> >> + if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
> >> config_base |= ARMV8_PMU_INCLUDE_EL2;
> >
> > This isn't quite what Will suggested.
> >
> > The idea was that userspace would read sysfs, then use that to determine
> > the correct exclusion parameters [1,2]. This logic was not expected to
> > change; it correctly validates whether we can provide what the user
> > requests.
>
> 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.
Will
On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <[email protected]> wrote:
> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
>> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <[email protected]> wrote:
>> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
>> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
>> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
>> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
>> >> when ran on VHE enabled platforms.
>> >>
>> >> Adding fix to consider only exclude_kernel attribute when kernel is
>> >> running in HYP. Also adding sysfs file to notify the bhehaviour
>> >> of attribute exclude_hv.
>> >>
>> >> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>> >> ---
>> >>
>> >> Changelog:
>> >>
>> >> V2:
>> >> - Changes as per Will Deacon's suggestion.
>> >>
>> >> V1: Initial patch
>> >>
>> >> arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
>> >> include/linux/perf/arm_pmu.h | 1 +
>> >> 2 files changed, 25 insertions(+), 4 deletions(-)
>> >>
>> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>> >>
>> >> if (attr->exclude_idle)
>> >> return -EPERM;
>> >> - if (is_kernel_in_hyp_mode() &&
>> >> - attr->exclude_kernel != attr->exclude_hv)
>> >> - return -EINVAL;
>> >> + if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
>> >> + config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >> if (attr->exclude_user)
>> >> config_base |= ARMV8_PMU_EXCLUDE_EL0;
>> >> if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
>> >> config_base |= ARMV8_PMU_EXCLUDE_EL1;
>> >> - if (!attr->exclude_hv)
>> >> + if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
>> >> config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >
>> > This isn't quite what Will suggested.
>> >
>> > The idea was that userspace would read sysfs, then use that to determine
>> > the correct exclusion parameters [1,2]. This logic was not expected to
>> > change; it correctly validates whether we can provide what the user
>> > requests.
>>
>> 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.
>
> Will
thanks
Ganapat
On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <[email protected]> wrote:
> > On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> >> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <[email protected]> wrote:
> >> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
> >> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
> >> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
> >> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
> >> >> when ran on VHE enabled platforms.
> >> >>
> >> >> Adding fix to consider only exclude_kernel attribute when kernel is
> >> >> running in HYP. Also adding sysfs file to notify the bhehaviour
> >> >> of attribute exclude_hv.
> >> >>
> >> >> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> >> >> ---
> >> >>
> >> >> Changelog:
> >> >>
> >> >> V2:
> >> >> - Changes as per Will Deacon's suggestion.
> >> >>
> >> >> V1: Initial patch
> >> >>
> >> >> arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
> >> >> include/linux/perf/arm_pmu.h | 1 +
> >> >> 2 files changed, 25 insertions(+), 4 deletions(-)
> >> >>
> >> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> >> >>
> >> >> if (attr->exclude_idle)
> >> >> return -EPERM;
> >> >> - if (is_kernel_in_hyp_mode() &&
> >> >> - attr->exclude_kernel != attr->exclude_hv)
> >> >> - return -EINVAL;
> >> >> + if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
> >> >> + config_base |= ARMV8_PMU_INCLUDE_EL2;
> >> >> if (attr->exclude_user)
> >> >> config_base |= ARMV8_PMU_EXCLUDE_EL0;
> >> >> if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
> >> >> config_base |= ARMV8_PMU_EXCLUDE_EL1;
> >> >> - if (!attr->exclude_hv)
> >> >> + if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
> >> >> config_base |= ARMV8_PMU_INCLUDE_EL2;
> >> >
> >> > This isn't quite what Will suggested.
> >> >
> >> > The idea was that userspace would read sysfs, then use that to determine
> >> > the correct exclusion parameters [1,2]. This logic was not expected to
> >> > change; it correctly validates whether we can provide what the user
> >> > requests.
> >>
> >> 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.
Will
Hi Will,
On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon <[email protected]> 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 <[email protected]> wrote:
>> > On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
>> >> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <[email protected]> wrote:
>> >> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
>> >> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
>> >> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
>> >> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
>> >> >> when ran on VHE enabled platforms.
>> >> >>
>> >> >> Adding fix to consider only exclude_kernel attribute when kernel is
>> >> >> running in HYP. Also adding sysfs file to notify the bhehaviour
>> >> >> of attribute exclude_hv.
>> >> >>
>> >> >> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>> >> >> ---
>> >> >>
>> >> >> Changelog:
>> >> >>
>> >> >> V2:
>> >> >> - Changes as per Will Deacon's suggestion.
>> >> >>
>> >> >> V1: Initial patch
>> >> >>
>> >> >> arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
>> >> >> include/linux/perf/arm_pmu.h | 1 +
>> >> >> 2 files changed, 25 insertions(+), 4 deletions(-)
>> >> >>
>> >> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>> >> >>
>> >> >> if (attr->exclude_idle)
>> >> >> return -EPERM;
>> >> >> - if (is_kernel_in_hyp_mode() &&
>> >> >> - attr->exclude_kernel != attr->exclude_hv)
>> >> >> - return -EINVAL;
>> >> >> + if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
>> >> >> + config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >> >> if (attr->exclude_user)
>> >> >> config_base |= ARMV8_PMU_EXCLUDE_EL0;
>> >> >> if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
>> >> >> config_base |= ARMV8_PMU_EXCLUDE_EL1;
>> >> >> - if (!attr->exclude_hv)
>> >> >> + if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
>> >> >> config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >> >
>> >> > This isn't quite what Will suggested.
>> >> >
>> >> > The idea was that userspace would read sysfs, then use that to determine
>> >> > the correct exclusion parameters [1,2]. This logic was not expected to
>> >> > change; it correctly validates whether we can provide what the user
>> >> > requests.
>> >>
>> >> 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
- the perf API documentation has to be updated for this
- 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.
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.
Hope I am mistake here, otherwise this does not sound like a good idea.
JC.
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 <[email protected]> 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 <[email protected]> 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.
Will
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 <[email protected]> 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 <[email protected]> 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.
If that is not acceptable (which seems to be the case - but I do not
see a reason for that), I think the better option for the application
is to check if the platform supports the mode exclusion it wants by
using the perf_event_open API itself.
Thanks,
JC.
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 <[email protected]> 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 <[email protected]> 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
On Thu, Apr 27, 2017 at 06:37:59PM +0100, Will Deacon wrote:
> 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 <[email protected]> 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 <[email protected]> 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.
If my understanding is correct, when is_kernel_in_hyp_mode() is true,
the kernel is in EL2 and there is no real hypervisor with hvc calls
from kernel. Ignoring the exclude_hv would be correct.
When kernel is in EL1, it would be correct to consider exclude_hv to
skip events in EL2 (reached with hvc).
I don't see the issue, can you please give more detail on the config
with unexpected behavior?
JC.
Hi guys,
On Fri, Apr 28, 2017 at 01:46:24PM +0000, Jayachandran C wrote:
> On Thu, Apr 27, 2017 at 06:37:59PM +0100, Will Deacon wrote:
> > > 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.
>
> If my understanding is correct, when is_kernel_in_hyp_mode() is true,
> the kernel is in EL2 and there is no real hypervisor with hvc calls
> from kernel. Ignoring the exclude_hv would be correct.
>
> When kernel is in EL1, it would be correct to consider exclude_hv to
> skip events in EL2 (reached with hvc).
>
> I don't see the issue, can you please give more detail on the config
> with unexpected behavior?
This got me thinking, so I tried to look at the history of exclude_hv. It
turns out it was added in 0475f9ea8e2c ("perf_counters: allow users to
count user, kernel and/or hypervisor events") for PowerPC, not x86 (where
this doesn't seem to be supported).
Notably, it looks like it's always ignored for the x86 CPU PMU, and ignored
on PowerPC when a hypervisor is not present. I think that backs up your
suggestion that we should ignore it when is_kernel_in_hyp_mode() is true.
In which case, I withdraw my objection to ignoring exclude_hv when running
in hyp mode, but please add a comment explaining the rationale!
Will