[v4,4/9] arm: perf: Remove Remove PMU locking

Commit Message

Julien Thierry July 17, 2019, 8:17 a.m. UTC

Since the PMU interrupt saves and restores the value of the selector
register, there is no need to serialize register accesses against the
interrupt contexts.

For operations that can be called with interrupts enabled, preemption
still needs to be disabled to ensure the programming of the PMU is
done on the expected CPU and not migrated mid-programming.

Signed-off-by: Julien Thierry <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: Peter Zijlstra <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Arnaldo Carvalho de Melo <[e-mail]>
Cc: Alexander Shishkin <[e-mail]>
Cc: Jiri Olsa <[e-mail]>
Cc: Namhyung Kim <[e-mail]>
Cc: Russell King <[e-mail]>
---
 arch/arm/kernel/perf_event_v7.c | 54 ++---------------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

--
1.9.1

Comments

Will Deacon Aug. 1, 2019, 1:06 p.m. UTC | #1

[extra 'Remove' in subject]

On Wed, Jul 17, 2019 at 09:17:07AM +0100, Julien Thierry wrote:
> Since the PMU interrupt saves and restores the value of the selector
> register, there is no need to serialize register accesses against the
> interrupt contexts.
> 
> For operations that can be called with interrupts enabled, preemption
> still needs to be disabled to ensure the programming of the PMU is
> done on the expected CPU and not migrated mid-programming.
> 
> Signed-off-by: Julien Thierry <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Peter Zijlstra <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: Arnaldo Carvalho de Melo <[e-mail]>
> Cc: Alexander Shishkin <[e-mail]>
> Cc: Jiri Olsa <[e-mail]>
> Cc: Namhyung Kim <[e-mail]>
> Cc: Russell King <[e-mail]>
> ---
>  arch/arm/kernel/perf_event_v7.c | 54 ++---------------------------------------
>  1 file changed, 2 insertions(+), 52 deletions(-)

I'm struggling to see why this patch is needed or, if it is, why we're not
doing something similar for v6 and xscale.

Will

Julien Thierry Aug. 2, 2019, 2:36 p.m. UTC | #2

On 01/08/2019 14:06, Will Deacon wrote:
> [extra 'Remove' in subject]
> 
> On Wed, Jul 17, 2019 at 09:17:07AM +0100, Julien Thierry wrote:
>> Since the PMU interrupt saves and restores the value of the selector
>> register, there is no need to serialize register accesses against the
>> interrupt contexts.
>>
>> For operations that can be called with interrupts enabled, preemption
>> still needs to be disabled to ensure the programming of the PMU is
>> done on the expected CPU and not migrated mid-programming.
>>
>> Signed-off-by: Julien Thierry <[e-mail]>
>> Cc: Will Deacon <[e-mail]>
>> Cc: Mark Rutland <[e-mail]>
>> Cc: Peter Zijlstra <[e-mail]>
>> Cc: Ingo Molnar <[e-mail]>
>> Cc: Arnaldo Carvalho de Melo <[e-mail]>
>> Cc: Alexander Shishkin <[e-mail]>
>> Cc: Jiri Olsa <[e-mail]>
>> Cc: Namhyung Kim <[e-mail]>
>> Cc: Russell King <[e-mail]>
>> ---
>>  arch/arm/kernel/perf_event_v7.c | 54 ++---------------------------------------
>>  1 file changed, 2 insertions(+), 52 deletions(-)
> 
> I'm struggling to see why this patch is needed or, if it is, why we're not
> doing something similar for v6 and xscale.

We can't do a similar things for v6 because it doesn't have set/clear
registers for event enabling/disabling. To do that on v6 we must do
read-modify-write, so we need to keep a spinlock.

For v7 we can do without it.

Thanks,

Patch

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index b7be2a3..9655127 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -882,10 +882,8 @@  static void armv7_pmnc_dump_regs(struct arm_pmu *cpu_pmu)

 static void armv7pmu_enable_event(struct perf_event *event)
 {
-	unsigned long flags;
 	struct hw_perf_event *hwc = &event->hw;
 	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
 	int idx = hwc->idx;

 	if (!armv7_pmnc_counter_valid(cpu_pmu, idx)) {
@@ -898,7 +896,6 @@  static void armv7pmu_enable_event(struct perf_event *event)
 	 * Enable counter and interrupt, and set the counter to count
 	 * the event that we're interested in.
 	 */
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);

 	/*
 	 * Disable counter
@@ -922,16 +919,12 @@  static void armv7pmu_enable_event(struct perf_event *event)
 	 * Enable counter
 	 */
 	armv7_pmnc_enable_counter(idx);
-
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static void armv7pmu_disable_event(struct perf_event *event)
 {
-	unsigned long flags;
 	struct hw_perf_event *hwc = &event->hw;
 	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
 	int idx = hwc->idx;

 	if (!armv7_pmnc_counter_valid(cpu_pmu, idx)) {
@@ -941,11 +934,6 @@  static void armv7pmu_disable_event(struct perf_event *event)
 	}

 	/*
-	 * Disable counter and interrupt
-	 */
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);
-
-	/*
 	 * Disable counter
 	 */
 	armv7_pmnc_disable_counter(idx);
@@ -954,8 +942,6 @@  static void armv7pmu_disable_event(struct perf_event *event)
 	 * Disable interrupt for this counter
 	 */
 	armv7_pmnc_disable_intens(idx);
-
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static irqreturn_t armv7pmu_handle_irq(struct arm_pmu *cpu_pmu)
@@ -1030,24 +1016,16 @@  static irqreturn_t armv7pmu_handle_irq(struct arm_pmu *cpu_pmu)

 static void armv7pmu_start(struct arm_pmu *cpu_pmu)
 {
-	unsigned long flags;
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
-
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+	WARN_ON_ONCE(preemptible());
 	/* Enable all counters */
 	armv7_pmnc_write(armv7_pmnc_read() | ARMV7_PMNC_E);
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static void armv7pmu_stop(struct arm_pmu *cpu_pmu)
 {
-	unsigned long flags;
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
-
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+	WARN_ON_ONCE(preemptible());
 	/* Disable all counters */
 	armv7_pmnc_write(armv7_pmnc_read() & ~ARMV7_PMNC_E);
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static int armv7pmu_get_event_idx(struct pmu_hw_events *cpuc,
@@ -1513,14 +1491,8 @@  static void krait_clearpmu(u32 config_base)

 static void krait_pmu_disable_event(struct perf_event *event)
 {
-	unsigned long flags;
 	struct hw_perf_event *hwc = &event->hw;
 	int idx = hwc->idx;
-	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
-
-	/* Disable counter and interrupt */
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);

 	/* Disable counter */
 	armv7_pmnc_disable_counter(idx);
@@ -1533,23 +1505,17 @@  static void krait_pmu_disable_event(struct perf_event *event)

 	/* Disable interrupt for this counter */
 	armv7_pmnc_disable_intens(idx);
-
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static void krait_pmu_enable_event(struct perf_event *event)
 {
-	unsigned long flags;
 	struct hw_perf_event *hwc = &event->hw;
 	int idx = hwc->idx;
-	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);

 	/*
 	 * Enable counter and interrupt, and set the counter to count
 	 * the event that we're interested in.
 	 */
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);

 	/* Disable counter */
 	armv7_pmnc_disable_counter(idx);
@@ -1569,8 +1535,6 @@  static void krait_pmu_enable_event(struct perf_event *event)

 	/* Enable counter */
 	armv7_pmnc_enable_counter(idx);
-
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static void krait_pmu_reset(void *info)
@@ -1846,14 +1810,8 @@  static void scorpion_clearpmu(u32 config_base)

 static void scorpion_pmu_disable_event(struct perf_event *event)
 {
-	unsigned long flags;
 	struct hw_perf_event *hwc = &event->hw;
 	int idx = hwc->idx;
-	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
-
-	/* Disable counter and interrupt */
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);

 	/* Disable counter */
 	armv7_pmnc_disable_counter(idx);
@@ -1866,23 +1824,17 @@  static void scorpion_pmu_disable_event(struct perf_event *event)

 	/* Disable interrupt for this counter */
 	armv7_pmnc_disable_intens(idx);
-
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static void scorpion_pmu_enable_event(struct perf_event *event)
 {
-	unsigned long flags;
 	struct hw_perf_event *hwc = &event->hw;
 	int idx = hwc->idx;
-	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
-	struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);

 	/*
 	 * Enable counter and interrupt, and set the counter to count
 	 * the event that we're interested in.
 	 */
-	raw_spin_lock_irqsave(&events->pmu_lock, flags);

 	/* Disable counter */
 	armv7_pmnc_disable_counter(idx);
@@ -1902,8 +1854,6 @@  static void scorpion_pmu_enable_event(struct perf_event *event)

 	/* Enable counter */
 	armv7_pmnc_enable_counter(idx);
-
-	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
 }

 static void scorpion_pmu_reset(void *info)