Skip to contentSkip to conversation

ARM: perf: remove unnecessary armpmu->stop

RFC PATCH New Ashwin Chaugule 2 participants 2 messages Aug 22, 2011 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 4B9A4BAF850C914D8DED94776A2C477E0B853A03@nasanexd01b.na.qualcomm.com

Ashwin Chaugule

author
#1 lore raw

RFC PATCH 8/15 ARM: perf: remove unnecessary armpmu->stop

Hello,

Quoted 24 lines
Quoted 24 lines

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 <[email protected]>
Reviewed-by Will Deacon <[email protected]>

---
arch/arm/kernel/perf_event.c | 1 -

1 files changed, 0 insertions(+), 1 deletions(-)
arch/arm/kernel/perf_event.c +0 -0
index 9d6ac99..5ce6c33 100644
@@ -396,7 +396,6 @@ armpmu_release_hardware(void)
 			free_irq(irq, NULL);
 	}
Quoted 2 lines

arch/arm/kernel/perf_event.c armpmu_release_hardware(void) · line 399

Quoted 2 lines
arch/arm/kernel/perf_event.c +0 -1
-	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

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#2 lore raw

RFC PATCH 8/15 ARM: perf: remove unnecessary armpmu->stop

Quoted 1 line · Ashwin Chaugule

Hello,

Hi Ashwin,

Quoted 31 lines
Quoted 24 lines
Quoted 24 lines

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 <[email protected]>
Reviewed-by Will Deacon <[email protected]>

---
arch/arm/kernel/perf_event.c | 1 -

1 files changed, 0 insertions(+), 1 deletions(-)
arch/arm/kernel/perf_event.c +0 -0
index 9d6ac99..5ce6c33 100644
@@ -396,7 +396,6 @@ armpmu_release_hardware(void)
 			free_irq(irq, NULL);
 	}
Quoted 2 lines

arch/arm/kernel/perf_event.c armpmu_release_hardware(void) · line 399

Quoted 2 lines
arch/arm/kernel/perf_event.c +0 -1
-	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() ?

Quoted 2 lines

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.

Quoted 1 line

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