[RFC PATCH 08/15] ARM: perf: remove unnecessary armpmu->stop

[RFC PATCH 08/15] ARM: perf: remove unnecessary armpmu->stop

[Mark Rutland]

As armpmu_disable will call armpmu->stop when the last event has been
removed, this is pointless and simply adds to the noise when debugging.
Additionally, due to this call occurring in a preemptible context, this
is problematic for per-cpu locking of PMU registers (where we will
attempt to access per-cpu spinlock for use with raw_spin_lock_irqsave).

This patch removes the call to armpmu->stop.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/perf_event.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 9d6ac99..5ce6c33 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -396,7 +396,6 @@ armpmu_release_hardware(void)
 			free_irq(irq, NULL);
 	}
 
-	armpmu->stop();
 	release_pmu(ARM_PMU_DEVICE_CPU);
 }
 
-- 
1.7.0.4

[RFC PATCH 08/15] ARM: perf: remove unnecessary armpmu->stop

[Mark Rutland]

As armpmu_disable will call armpmu->stop when the last event has been
removed, this is pointless and simply adds to the noise when debugging.
Additionally, due to this call occurring in a preemptible context, this
is problematic for per-cpu locking of PMU registers (where we will
attempt to access per-cpu spinlock for use with raw_spin_lock_irqsave).

This patch removes the call to armpmu->stop.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/perf_event.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 9d6ac99..5ce6c33 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -396,7 +396,6 @@ armpmu_release_hardware(void)
 			free_irq(irq, NULL);
 	}
 
-	armpmu->stop();
 	release_pmu(ARM_PMU_DEVICE_CPU);
 }
 
-- 
1.7.0.4

[RFC PATCH 08/15] ARM: perf: remove unnecessary armpmu->stop

[Ashwin Chaugule]

Hello,

>> From: Mark Rutland 
>>
>> As armpmu_disable will call armpmu->stop when the last event has been
>> removed, this is pointless and simply adds to the noise when debugging.
>> Additionally, due to this call occurring in a preemptible context, this
>> is problematic for per-cpu locking of PMU registers (where we will
>> attempt to access per-cpu spinlock for use with raw_spin_lock_irqsave).
>>
>> This patch removes the call to armpmu->stop.
>>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> Reviewed-by: Will Deacon <will.deacon@arm.com>
>> ---
>> arch/arm/kernel/perf_event.c |    1 -
>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
>> index 9d6ac99..5ce6c33 100644
>> --- a/arch/arm/kernel/perf_event.c
>> +++ b/arch/arm/kernel/perf_event.c
>> @@ -396,7 +396,6 @@ armpmu_release_hardware(void)
>> 			free_irq(irq, NULL);
>> 	}
>>
>> -	armpmu->stop();
>> 	release_pmu(ARM_PMU_DEVICE_CPU);


Makes sense. On a similar note, I've been wondering why we need to loop
through all events in armpmu_enable() ?

Wouldn't the event->add call have taken care of armpmu->enable prior to
calling armpmu_enable() ?

I think we just need an armpmu->start(). What are your thoughts ?

Cheers,
Ashwin

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC PATCH 08/15] ARM: perf: remove unnecessary armpmu->stop

[Will Deacon]

On Mon, Aug 22, 2011 at 07:36:18PM +0100, Ashwin Chaugule wrote:
> Hello,

Hi Ashwin,

> >> From: Mark Rutland 
> >>
> >> As armpmu_disable will call armpmu->stop when the last event has been
> >> removed, this is pointless and simply adds to the noise when debugging.
> >> Additionally, due to this call occurring in a preemptible context, this
> >> is problematic for per-cpu locking of PMU registers (where we will
> >> attempt to access per-cpu spinlock for use with raw_spin_lock_irqsave).
> >>
> >> This patch removes the call to armpmu->stop.
> >>
> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> >> Reviewed-by: Will Deacon <will.deacon@arm.com>
> >> ---
> >> arch/arm/kernel/perf_event.c |    1 -
> >> 1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> >> index 9d6ac99..5ce6c33 100644
> >> --- a/arch/arm/kernel/perf_event.c
> >> +++ b/arch/arm/kernel/perf_event.c
> >> @@ -396,7 +396,6 @@ armpmu_release_hardware(void)
> >> 			free_irq(irq, NULL);
> >> 	}
> >>
> >> -	armpmu->stop();
> >> 	release_pmu(ARM_PMU_DEVICE_CPU);
> 
> 
> Makes sense. On a similar note, I've been wondering why we need to loop
> through all events in armpmu_enable() ?
> 
> Wouldn't the event->add call have taken care of armpmu->enable prior to
> calling armpmu_enable() ?

Hmm, I think you might be right. armpmu->enable is called from armpmu_start,
which is called from armpmu->add whenever a new event is added. The current
redundancy is likely a hangover from before we had the PERF_EF_*/PERF_HES_*
flags.

> I think we just need an armpmu->start(). What are your thoughts ?

Yes, conditional on armpmu->num_events > 0. I'll give it a spin and add it to
my perf-updates branch if I don't see any regressions.

Cheers,

Will