| Message ID | 1562596377-33196-3-git-send-email-julien.thierry@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm_pmu: Use NMI for perf interrupt | expand |
On Mon, Jul 08, 2019 at 03:32:50PM +0100, Julien Thierry wrote: > Since the PMU driver uses direct registers for counter > setup/manipulation, locking around these operations is no longer needed. > > 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. [...] > static void armv8pmu_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); > + preempt_disable(); > /* Enable all counters */ > armv8pmu_pmcr_write(armv8pmu_pmcr_read() | ARMV8_PMU_PMCR_E); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > + preempt_enable(); > } > > static void armv8pmu_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); > + preempt_disable(); > /* Disable all counters */ > armv8pmu_pmcr_write(armv8pmu_pmcr_read() & ~ARMV8_PMU_PMCR_E); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > + preempt_enable(); > } I think that we'd have bigger problems if these could be called in preemptible context, since we couldn't guarantee which HW PMU instance they'd operate on. I also thought that the interrupt disable/enable was a hold-over from the old days of perf core, and these days all of the synchronous operations are held with the pmu ctx lock held (and interrupts disabled). Do you have an example of when these are called with interrupts enabled? Or in a preemptible context? Thanks, Mark. > > static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu) > -- > 1.9.1
On 08/07/2019 16:03, Mark Rutland wrote: > On Mon, Jul 08, 2019 at 03:32:50PM +0100, Julien Thierry wrote: >> Since the PMU driver uses direct registers for counter >> setup/manipulation, locking around these operations is no longer needed. >> >> 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. > > [...] > >> static void armv8pmu_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); >> + preempt_disable(); >> /* Enable all counters */ >> armv8pmu_pmcr_write(armv8pmu_pmcr_read() | ARMV8_PMU_PMCR_E); >> - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); >> + preempt_enable(); >> } >> >> static void armv8pmu_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); >> + preempt_disable(); >> /* Disable all counters */ >> armv8pmu_pmcr_write(armv8pmu_pmcr_read() & ~ARMV8_PMU_PMCR_E); >> - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); >> + preempt_enable(); >> } > > I think that we'd have bigger problems if these could be called in > preemptible context, since we couldn't guarantee which HW PMU instance > they'd operate on. > > I also thought that the interrupt disable/enable was a hold-over from > the old days of perf core, and these days all of the synchronous > operations are held with the pmu ctx lock held (and interrupts > disabled). > > Do you have an example of when these are called with interrupts enabled? > Or in a preemptible context? > I must admit that not seeing mention of the pmu_enable/disable callbacks being called with preemption disabled in perf_event.h, I assumed the worst. But looking at it, it does look like they are always called either with interrupts or preemption disabled, and the x86 implementation doesn't seem to worry about being preempted in those callbacks. I guess I can get rid of the preempt_enable/disable. Thanks,
On Mon, Jul 08, 2019 at 04:34:01PM +0100, Julien Thierry wrote: > > > On 08/07/2019 16:03, Mark Rutland wrote: > > On Mon, Jul 08, 2019 at 03:32:50PM +0100, Julien Thierry wrote: > >> Since the PMU driver uses direct registers for counter > >> setup/manipulation, locking around these operations is no longer needed. > >> > >> 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. > > > > [...] > > > >> static void armv8pmu_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); > >> + preempt_disable(); > >> /* Enable all counters */ > >> armv8pmu_pmcr_write(armv8pmu_pmcr_read() | ARMV8_PMU_PMCR_E); > >> - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > >> + preempt_enable(); > >> } > >> > >> static void armv8pmu_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); > >> + preempt_disable(); > >> /* Disable all counters */ > >> armv8pmu_pmcr_write(armv8pmu_pmcr_read() & ~ARMV8_PMU_PMCR_E); > >> - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > >> + preempt_enable(); > >> } > > > > I think that we'd have bigger problems if these could be called in > > preemptible context, since we couldn't guarantee which HW PMU instance > > they'd operate on. > > > > I also thought that the interrupt disable/enable was a hold-over from > > the old days of perf core, and these days all of the synchronous > > operations are held with the pmu ctx lock held (and interrupts > > disabled). > > > > Do you have an example of when these are called with interrupts enabled? > > Or in a preemptible context? > > I must admit that not seeing mention of the pmu_enable/disable callbacks > being called with preemption disabled in perf_event.h, I assumed the > worst. But looking at it, it does look like they are always called > either with interrupts or preemption disabled, and the x86 > implementation doesn't seem to worry about being preempted in those > callbacks. > > I guess I can get rid of the preempt_enable/disable. Yes please! As an aside, this is something I'd like to explicitly annotate on functions so that we could statically and dnyamically test it. For now I'm happy to rely on the semantics documented in perf_event.h. Thanks, Mark.
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index 7759f8a..878c142 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -677,15 +677,10 @@ static inline u32 armv8pmu_getreset_flags(void) static void armv8pmu_enable_event(struct perf_event *event) { - unsigned long flags; - 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 @@ -706,21 +701,10 @@ static void armv8pmu_enable_event(struct perf_event *event) * Enable counter */ armv8pmu_enable_event_counter(event); - - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); } static void armv8pmu_disable_event(struct perf_event *event) { - unsigned long flags; - 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 */ @@ -730,30 +714,22 @@ static void armv8pmu_disable_event(struct perf_event *event) * Disable interrupt for this counter */ armv8pmu_disable_event_irq(event); - - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); } static void armv8pmu_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); + preempt_disable(); /* Enable all counters */ armv8pmu_pmcr_write(armv8pmu_pmcr_read() | ARMV8_PMU_PMCR_E); - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); + preempt_enable(); } static void armv8pmu_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); + preempt_disable(); /* Disable all counters */ armv8pmu_pmcr_write(armv8pmu_pmcr_read() & ~ARMV8_PMU_PMCR_E); - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); + preempt_enable(); } static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu)
Since the PMU driver uses direct registers for counter setup/manipulation, locking around these operations is no longer needed. 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 <julien.thierry@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> --- arch/arm64/kernel/perf_event.c | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) -- 1.9.1