Re: Unfair qspinlocks on ARM64 without LSE atomics => 3ms delay in interrupt handling

[Catalin Marinas]

On Fri, Mar 24, 2023 at 08:43:38AM +0000, Bouska, Zdenek wrote:
> I have seen ~3 ms delay in interrupt handling on ARM64.
> 
> I have traced it down to raw_spin_lock() call in handle_irq_event() in
> kernel/irq/handle.c:
> 
> irqreturn_t handle_irq_event(struct irq_desc *desc)
> {
>     irqreturn_t ret;
> 
>     desc->istate &= ~IRQS_PENDING;
>     irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>     raw_spin_unlock(&desc->lock);
> 
>     ret = handle_irq_event_percpu(desc);
> 
> --> raw_spin_lock(&desc->lock);
>     irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>     return ret;
> }
> 
> It took ~3 ms for this raw_spin_lock() to lock.

That's quite a large indeed.

> During this time irq_finalize_oneshot() from kernel/irq/manage.c locks and
> unlocks the same raw spin lock more than 1000 times:
> 
> static void irq_finalize_oneshot(struct irq_desc *desc,
>                  struct irqaction *action)
> {
>     if (!(desc->istate & IRQS_ONESHOT) ||
>         action->handler == irq_forced_secondary_handler)
>         return;
> again:
>     chip_bus_lock(desc);
> --> raw_spin_lock_irq(&desc->lock);
> 
>     /*
>      * Implausible though it may be we need to protect us against
>      * the following scenario:
>      *
>      * The thread is faster done than the hard interrupt handler
>      * on the other CPU. If we unmask the irq line then the
>      * interrupt can come in again and masks the line, leaves due
>      * to IRQS_INPROGRESS and the irq line is masked forever.
>      *
>      * This also serializes the state of shared oneshot handlers
>      * versus "desc->threads_oneshot |= action->thread_mask;" in
>      * irq_wake_thread(). See the comment there which explains the
>      * serialization.
>      */
>     if (unlikely(irqd_irq_inprogress(&desc->irq_data))) {
> -->     raw_spin_unlock_irq(&desc->lock);
>         chip_bus_sync_unlock(desc);
>         cpu_relax();
>         goto again;
>     }

So this path is hammering the desc->lock location and another CPU cannot
change it. As you found, the problem is not the spinlock algorithm but
the atomic primitives. The LDXR/STXR constructs on arm64 are known to
have this issue with STXR failing indefinitely. raw_spin_unlock() simply
does an STLR and this clears the exclusive monitor that the other CPU
may have set with LDXR but before the STXR. The queued spinlock only
provides fairness if the CPU manages to get in the queue.

> So I confirmed that atomic operations from
> arch/arm64/include/asm/atomic_ll_sc.h can be quite slow when they are
> contested from second CPU.
> 
> Do you think that it is possible to create fair qspinlock implementation
> on top of atomic instructions supported by ARM64 version 8 (no LSE atomic
> instructions) without compromising performance in the uncontested case?
> For example ARM64 could have custom queued_fetch_set_pending_acquire
> implementation same as x86 has in arch/x86/include/asm/qspinlock.h. Is the
> retry loop in irq_finalize_oneshot() ok together with the current ARM64
> cpu_relax() implementation for processor with no LSE atomic instructions?

So is the queued_fetch_set_pending_acquire() where it gets stuck or the
earlier atomic_try_cmpxchg_acquire() before entering on the slow path? I
guess both can fail in a similar way.

A longer cpu_relax() here would improve things (on arm64 this function
is a no-op) but maybe Thomas or Will have a better idea.

-- 
Catalin