Hi Mark, On 6/22/20 3:19 PM, Mark Rutland wrote: > On Wed, Jun 17, 2020 at 12:38:48PM +0100, Alexandru Elisei wrote: >> From: Julien Thierry <[email protected]> >> >> perf_event_overflow() can queue an irq_work on the current PE, which is >> executed via an IPI. Move the processing of the irq_work from the PMU IRQ >> handler to the IPI handler, which gets executed immediately afterwards. >> >> This also makes the IRQ handler NMI safe, because it removes the call to >> irq_work_run(). > It wasn't entirely clear to me what the situation was today, and why > this was sound. How about the following to spell that out more > explicitly: > > | When handling events armv8pmu_handle_irq() calls > | perf_event_overflow(), and subsequently calls irq_work_run() to handle > | any work queued by perf_event_overflow(). As perf_event_overflow() > | raises IPI_IRQ_WORK when queing the work, this isn't strictly > | necessary and the work could be handled as part of the IPI_IRQ_WORK > | handler. > | > | In the common case the IPI handler will run immediately after the PMU > | IRQ handler, and where the PE is heavily loaded with interrupts other > | handlers may run first, widening the window where some counters are > | disabled. > | > | In practice this window is unlikely to be a significant issue, and > | removing the call to irq_work_run() would make the PMU IRQ handler NMI > | safe in addition to making it simpler, so let's do that. > > Thanks, > Mark.
That is much better than my commit message, I will definitely update it with your suggestion. Thanks, Alex > >> Cc: Julien Thierry <[email protected]> >> Cc: Will Deacon <[email protected]> >> Cc: Mark Rutland <[email protected]> >> Cc: Peter Zijlstra <[email protected]> >> Cc: Ingo Molnar <[email protected]> >> Cc: Arnaldo Carvalho de Melo <[email protected]> >> Cc: Alexander Shishkin <[email protected]> >> Cc: Jiri Olsa <[email protected]> >> Cc: Namhyung Kim <[email protected]> >> Cc: Catalin Marinas <[email protected]> >> Signed-off-by: Julien Thierry <[email protected]> >> [Reworded commit] >> Signed-off-by: Alexandru Elisei <[email protected]> >> --- >> arch/arm64/kernel/perf_event.c | 14 +++++--------- >> 1 file changed, 5 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >> index a6195022be7d..cf1d92030790 100644 >> --- a/arch/arm64/kernel/perf_event.c >> +++ b/arch/arm64/kernel/perf_event.c >> @@ -750,20 +750,16 @@ static irqreturn_t armv8pmu_handle_irq(struct arm_pmu >> *cpu_pmu) >> if (!armpmu_event_set_period(event)) >> continue; >> >> + /* >> + * Perf event overflow will queue the processing of the event as >> + * an irq_work which will be taken care of in the handling of >> + * IPI_IRQ_WORK. >> + */ >> if (perf_event_overflow(event, &data, regs)) >> cpu_pmu->disable(event); >> } >> armv8pmu_start(cpu_pmu); >> >> - /* >> - * Handle the pending perf events. >> - * >> - * Note: this call *must* be run with interrupts disabled. For >> - * platforms that can have the PMU interrupts raised as an NMI, this >> - * will not work. >> - */ >> - irq_work_run(); >> - >> return IRQ_HANDLED; >> } >> >> -- >> 2.27.0 >>
