| Message ID | 20200617113851.607706-4-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm_pmu: Use NMI for perf interrupt | expand |
Quoting Alexandru Elisei (2020-06-17 04:38:47) > From: Julien Thierry <julien.thierry@arm.com> > > The PMU is disabled and enabled, and the counters are programmed from > contexts where interrupts or preemption is disabled. > > The functions to toggle the PMU and to program the PMU counters access the > registers directly and don't access data modified by the interrupt handler. > That, and the fact that they're always called from non-preemptible > contexts, means that we don't need to disable interrupts or use a spinlock. Maybe we should add a lockdep assertion that the code isn't preemptible? I.e. add a cant_sleep() call? Or is it more that we don't need locking because we're just doing register accesses and don't need to protect those accesses from each other?
Hi, On 6/17/20 9:17 PM, Stephen Boyd wrote: > Quoting Alexandru Elisei (2020-06-17 04:38:47) >> From: Julien Thierry <julien.thierry@arm.com> >> >> The PMU is disabled and enabled, and the counters are programmed from >> contexts where interrupts or preemption is disabled. >> >> The functions to toggle the PMU and to program the PMU counters access the >> registers directly and don't access data modified by the interrupt handler. >> That, and the fact that they're always called from non-preemptible >> contexts, means that we don't need to disable interrupts or use a spinlock. > Maybe we should add a lockdep assertion that the code isn't preemptible? > I.e. add a cant_sleep() call? Or is it more that we don't need locking > because we're just doing register accesses and don't need to protect > those accesses from each other? It's both. The spinlocks were there to protect the functions from being preempted and possibly migrated to another CPU, and from being interrupted by the PMU irq handler. There was no data race with the interrupt handler, but before the previous patch ("arm64: perf: Avoid PMXEV* indirection"), in order to read/write/program a counter, one had to write the counter number to a counter selection register, and then write/read the desired value from another register. This was done from both the armv8pmu_{enable,disable}_event() functions and the irq handler, and the spinlock was necessary. Now that we can access a counter using a single register access, there's no need to protect the functions from being interrupted by the IRQ handler. As for armv8pmu_{start,stop}(), they consist of one register write, so it's also safe for the irq handler to interrupt them. For the preemption part of the locking. The armv8pmu_{enable,disable}_event(), when called by the perf core code via the pmu->{start,stop,add,del} callbacks, are guaranteed to be called with IRQs and preemption disabled, as per the comment in include/linux/perf_event.h. They are also called from the arm_pmu driver by the CPU PM notifiers, which should also be executed with interrupts disabled. Should we check here that the top level code respects these guarantees? The armv8pmu_{start,stop}() functions are called from the irq handler, so we're safe from preemption in this case. They are also called via pmu->pmu_{enable,disable} callbacks, and I didn't find an explicit contract regarding preemption in include/linux/perf_event.h. I've checked the other call sites, and I didn't find any instances where they are called with preemption enabled, which makes sense as we don't want to disable the PMU on a another CPU by accident. I would be inclined to add cant_sleep() calls to armv8pmu_{start,stop}(). In the previous iteration, there were WARN_ONs in these functions, and Will said [1] they can be removed because they are per-CPU operations. Will, what do you think about adding the lockdep assertions? [1] https://www.spinics.net/lists/arm-kernel/msg745161.html Thanks, Alex
Quoting Alexandru Elisei (2020-06-18 03:51:31) > Hi, > > On 6/17/20 9:17 PM, Stephen Boyd wrote: > > Quoting Alexandru Elisei (2020-06-17 04:38:47) > >> From: Julien Thierry <julien.thierry@arm.com> > >> > >> The PMU is disabled and enabled, and the counters are programmed from > >> contexts where interrupts or preemption is disabled. > >> > >> The functions to toggle the PMU and to program the PMU counters access the > >> registers directly and don't access data modified by the interrupt handler. > >> That, and the fact that they're always called from non-preemptible > >> contexts, means that we don't need to disable interrupts or use a spinlock. > > Maybe we should add a lockdep assertion that the code isn't preemptible? > > I.e. add a cant_sleep() call? Or is it more that we don't need locking > > because we're just doing register accesses and don't need to protect > > those accesses from each other? > > It's both. The spinlocks were there to protect the functions from being preempted > and possibly migrated to another CPU, and from being interrupted by the PMU irq > handler. > > There was no data race with the interrupt handler, but before the previous patch > ("arm64: perf: Avoid PMXEV* indirection"), in order to read/write/program a > counter, one had to write the counter number to a counter selection register, and > then write/read the desired value from another register. This was done from both > the armv8pmu_{enable,disable}_event() functions and the irq handler, and the > spinlock was necessary. Now that we can access a counter using a single register > access, there's no need to protect the functions from being interrupted by the IRQ > handler. As for armv8pmu_{start,stop}(), they consist of one register write, so > it's also safe for the irq handler to interrupt them. > > For the preemption part of the locking. The armv8pmu_{enable,disable}_event(), > when called by the perf core code via the pmu->{start,stop,add,del} callbacks, are > guaranteed to be called with IRQs and preemption disabled, as per the comment in > include/linux/perf_event.h. They are also called from the arm_pmu driver by the > CPU PM notifiers, which should also be executed with interrupts disabled. Should > we check here that the top level code respects these guarantees? > > The armv8pmu_{start,stop}() functions are called from the irq handler, so we're > safe from preemption in this case. They are also called via > pmu->pmu_{enable,disable} callbacks, and I didn't find an explicit contract > regarding preemption in include/linux/perf_event.h. I've checked the other call > sites, and I didn't find any instances where they are called with preemption > enabled, which makes sense as we don't want to disable the PMU on a another CPU by > accident. If they're all callbacks then it's overkill to add this. Presumably it is better to enforce this wherever the callbacks are called from so as to not litter the callee with random cant_sleep() calls. Probably best to ignore my suggestion. > > I would be inclined to add cant_sleep() calls to armv8pmu_{start,stop}(). In the > previous iteration, there were WARN_ONs in these functions, and Will said [1] they > can be removed because they are per-CPU operations. Will, what do you think about > adding the lockdep assertions? > > [1] https://www.spinics.net/lists/arm-kernel/msg745161.html > If I read it correctly Will is saying the same thing in that thread.
On Fri, Jun 19, 2020 at 01:29:59AM -0700, Stephen Boyd wrote: > Quoting Alexandru Elisei (2020-06-18 03:51:31) > > The armv8pmu_{start,stop}() functions are called from the irq handler, so we're > > safe from preemption in this case. They are also called via > > pmu->pmu_{enable,disable} callbacks, and I didn't find an explicit contract > > regarding preemption in include/linux/perf_event.h. I've checked the other call > > sites, and I didn't find any instances where they are called with preemption > > enabled, which makes sense as we don't want to disable the PMU on a another CPU by > > accident. > > If they're all callbacks then it's overkill to add this. Presumably it > is better to enforce this wherever the callbacks are called from so as > to not litter the callee with random cant_sleep() calls. Probably best > to ignore my suggestion. > > > > > I would be inclined to add cant_sleep() calls to armv8pmu_{start,stop}(). In the > > previous iteration, there were WARN_ONs in these functions, and Will said [1] they > > can be removed because they are per-CPU operations. Will, what do you think about > > adding the lockdep assertions? > > > > [1] https://www.spinics.net/lists/arm-kernel/msg745161.html > > > > If I read it correctly Will is saying the same thing in that thread. Right, in the cases where perf core already relies on things not being preemptible, we don't need to add extra checks. Will
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index e95b5ca70a53..a6195022be7d 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -647,15 +647,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 @@ -678,21 +673,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 */ @@ -702,30 +686,18 @@ 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); /* Enable all counters */ armv8pmu_pmcr_write(armv8pmu_pmcr_read() | ARMV8_PMU_PMCR_E); - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); } 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); /* Disable all counters */ armv8pmu_pmcr_write(armv8pmu_pmcr_read() & ~ARMV8_PMU_PMCR_E); - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); } static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu)