[PATCH] perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

[PATCH] perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

[Rob Herring (Arm)]

If the user has requested a counting threshold for the CPU cycles event,
then the fixed cycle counter can't be assigned as it lacks threshold
support. Currently, the thresholds will work or not randomly depending
on which counter the event is assigned.

While using thresholds for CPU cycles doesn't make much sense, it can be
useful for testing purposes.

Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold")
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/perf/arm_pmuv3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
index 23fa6c5da82c..2612be29ee23 100644
--- a/drivers/perf/arm_pmuv3.c
+++ b/drivers/perf/arm_pmuv3.c
@@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
 	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
 	unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
+	bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH);
 
 	/* Always prefer to place a cycle counter into the cycle counter. */
-	if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
+	if ((evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) && !has_threshold) {
 		if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask))
 			return ARMV8_IDX_CYCLE_COUNTER;
 		else if (armv8pmu_event_is_64bit(event) &&
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

[James Clark]

On 11/06/2024 16:50, Rob Herring (Arm) wrote:
> If the user has requested a counting threshold for the CPU cycles event,
> then the fixed cycle counter can't be assigned as it lacks threshold
> support. Currently, the thresholds will work or not randomly depending
> on which counter the event is assigned.
> 
> While using thresholds for CPU cycles doesn't make much sense, it can be
> useful for testing purposes.
> 
> Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold")
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/perf/arm_pmuv3.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 23fa6c5da82c..2612be29ee23 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
>  	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>  	struct hw_perf_event *hwc = &event->hw;
>  	unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
> +	bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH);

I was going to say doesn't it need to be (ARMV8_PMU_EVTYPE_TH |
ARMV8_PMU_EVTYPE_TC) for it to give the same results as the hardware.
But then I saw we only enable it if TH != 0, even if TC is set. And now
I'm wondering if I inadvertently disabled a useful combination of options.

The Arm ARM says it's only completely disabled when both TC and TH are 0.

>  
>  	/* Always prefer to place a cycle counter into the cycle counter. */
> -	if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
> +	if ((evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) && !has_threshold) {
>  		if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask))
>  			return ARMV8_IDX_CYCLE_COUNTER;
>  		else if (armv8pmu_event_is_64bit(event) &&

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

[Will Deacon]

On Tue, Jun 11, 2024 at 09:50:12AM -0600, Rob Herring (Arm) wrote:
> If the user has requested a counting threshold for the CPU cycles event,
> then the fixed cycle counter can't be assigned as it lacks threshold
> support. Currently, the thresholds will work or not randomly depending
> on which counter the event is assigned.
> 
> While using thresholds for CPU cycles doesn't make much sense, it can be
> useful for testing purposes.
> 
> Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold")
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/perf/arm_pmuv3.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 23fa6c5da82c..2612be29ee23 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
>  	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>  	struct hw_perf_event *hwc = &event->hw;
>  	unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
> +	bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH);

Just a nit, but I don't think you need the '!!' here.

Will

Re: [PATCH] perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

[James Clark]

On 11/06/2024 17:13, James Clark wrote:
> 
> 
> On 11/06/2024 16:50, Rob Herring (Arm) wrote:
>> If the user has requested a counting threshold for the CPU cycles event,
>> then the fixed cycle counter can't be assigned as it lacks threshold
>> support. Currently, the thresholds will work or not randomly depending
>> on which counter the event is assigned.
>>
>> While using thresholds for CPU cycles doesn't make much sense, it can be
>> useful for testing purposes.
>>
>> Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold")
>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>> ---
>>  drivers/perf/arm_pmuv3.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index 23fa6c5da82c..2612be29ee23 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
>>  	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>>  	struct hw_perf_event *hwc = &event->hw;
>>  	unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
>> +	bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH);
> 
> I was going to say doesn't it need to be (ARMV8_PMU_EVTYPE_TH |
> ARMV8_PMU_EVTYPE_TC) for it to give the same results as the hardware.
> But then I saw we only enable it if TH != 0, even if TC is set. And now
> I'm wondering if I inadvertently disabled a useful combination of options.
> 
> The Arm ARM says it's only completely disabled when both TC and TH are 0.
> 

If it's easy it might be worth adding a helper function for
has_threshold() that's used in both places. That way if or when this
issue gets fixed up it doesn't break here.

>>  
>>  	/* Always prefer to place a cycle counter into the cycle counter. */
>> -	if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
>> +	if ((evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) && !has_threshold) {
>>  		if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask))
>>  			return ARMV8_IDX_CYCLE_COUNTER;
>>  		else if (armv8pmu_event_is_64bit(event) &&

Re: [PATCH] perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

[Rob Herring]

On Tue, Jun 18, 2024 at 10:30 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 11/06/2024 17:13, James Clark wrote:
> >
> >
> > On 11/06/2024 16:50, Rob Herring (Arm) wrote:
> >> If the user has requested a counting threshold for the CPU cycles event,
> >> then the fixed cycle counter can't be assigned as it lacks threshold
> >> support. Currently, the thresholds will work or not randomly depending
> >> on which counter the event is assigned.
> >>
> >> While using thresholds for CPU cycles doesn't make much sense, it can be
> >> useful for testing purposes.
> >>
> >> Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold")
> >> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> >> ---
> >>  drivers/perf/arm_pmuv3.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> >> index 23fa6c5da82c..2612be29ee23 100644
> >> --- a/drivers/perf/arm_pmuv3.c
> >> +++ b/drivers/perf/arm_pmuv3.c
> >> @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
> >>      struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
> >>      struct hw_perf_event *hwc = &event->hw;
> >>      unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
> >> +    bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH);
> >
> > I was going to say doesn't it need to be (ARMV8_PMU_EVTYPE_TH |
> > ARMV8_PMU_EVTYPE_TC) for it to give the same results as the hardware.
> > But then I saw we only enable it if TH != 0, even if TC is set. And now
> > I'm wondering if I inadvertently disabled a useful combination of options.
> >
> > The Arm ARM says it's only completely disabled when both TC and TH are 0.
> >
>
> If it's easy it might be worth adding a helper function for
> has_threshold() that's used in both places. That way if or when this
> issue gets fixed up it doesn't break here.

The other place being in armv8pmu_set_event_filter()? A helper doesn't
help there because that looks at the attr value, not config_base.

Rob

Re: [PATCH] perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

[Rob Herring]

On Tue, Jun 18, 2024 at 9:41 AM Will Deacon <will@kernel.org> wrote:
>
> On Tue, Jun 11, 2024 at 09:50:12AM -0600, Rob Herring (Arm) wrote:
> > If the user has requested a counting threshold for the CPU cycles event,
> > then the fixed cycle counter can't be assigned as it lacks threshold
> > support. Currently, the thresholds will work or not randomly depending
> > on which counter the event is assigned.
> >
> > While using thresholds for CPU cycles doesn't make much sense, it can be
> > useful for testing purposes.
> >
> > Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold")
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> >  drivers/perf/arm_pmuv3.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> > index 23fa6c5da82c..2612be29ee23 100644
> > --- a/drivers/perf/arm_pmuv3.c
> > +++ b/drivers/perf/arm_pmuv3.c
> > @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
> >       struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
> >       struct hw_perf_event *hwc = &event->hw;
> >       unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
> > +     bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH);
>
> Just a nit, but I don't think you need the '!!' here.

Right, I guess since bool is a first class type in C9X we don't have
to worry about truncation. Old habits...

Rob