diff mbox series

perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold

Message ID 20240611155012.2286044-1-robh@kernel.org (mailing list archive)
State New, archived
Headers show
Series perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold | expand

Commit Message

Rob Herring (Arm) June 11, 2024, 3:50 p.m. UTC
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(-)

Comments

James Clark June 11, 2024, 4:13 p.m. UTC | #1
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) &&
Will Deacon June 18, 2024, 3:41 p.m. UTC | #2
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
James Clark June 18, 2024, 4:30 p.m. UTC | #3
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) &&
Rob Herring (Arm) June 24, 2024, 4:58 p.m. UTC | #4
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
Rob Herring (Arm) June 24, 2024, 8:14 p.m. UTC | #5
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
diff mbox series

Patch

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) &&