Re: [patch V9 21/39] x86/irq: Convey vector as argument and not in ptregs

[Brian Gerst <brgerst@gmail.com>]

On Tue, Aug 25, 2020 at 8:04 PM Alexander Graf <graf@amazon.com> wrote:
>
> Hi Andy,
>
> On 26.08.20 01:41, Andy Lutomirski wrote:
> >
> > On Tue, Aug 25, 2020 at 4:18 PM Alexander Graf <graf@amazon.com> wrote:
> >>
> >> Hi Thomas,
> >>
> >> On 25.08.20 12:28, Thomas Gleixner wrote:
> >>> void irq_complete_move(struct irq_cfg *cfg)
> > {
> >          __irq_complete_move(cfg, ~get_irq_regs()->orig_ax);
> > }
> >
> >>> Alex,
> >>>
> >>> On Mon, Aug 24 2020 at 19:29, Alexander Graf wrote:
> >>>> I'm currently trying to understand a performance regression with
> >>>> ScyllaDB on i3en.3xlarge (KVM based VM on Skylake) which we reliably
> >>>> bisected down to this commit:
> >>>>
> >>>>      https://github.com/scylladb/scylla/issues/7036
> >>>>
> >>>> What we're seeing is that syscalls such as membarrier() take forever
> >>>> (0-10 µs would be normal):
> >>> ...
> >>>> That again seems to stem from a vastly slowed down
> >>>> smp_call_function_many_cond():
> >>>>
> >>>> Samples: 218K of event 'cpu-clock', 4000 Hz
> >>>> Overhead  Shared Object        Symbol
> >>>>      94.51%  [kernel]             [k] smp_call_function_many_cond
> >>>>       0.76%  [kernel]             [k] __do_softirq
> >>>>       0.32%  [kernel]             [k] native_queued_spin_lock_slowpath
> >>>> [...]
> >>>>
> >>>> which is stuck in
> >>>>
> >>>>           │     csd_lock_wait():
> >>>>           │             smp_cond_load_acquire(&csd->flags, !(VAL &
> >>>>      0.00 │       mov    0x8(%rcx),%edx
> >>>>      0.00 │       and    $0x1,%edx
> >>>>           │     ↓ je     2b9
> >>>>           │     rep_nop():
> >>>>      0.70 │2af:   pause
> >>>>           │     csd_lock_wait():
> >>>>     92.82 │       mov    0x8(%rcx),%edx
> >>>>      6.48 │       and    $0x1,%edx
> >>>>      0.00 │     ↑ jne    2af
> >>>>      0.00 │2b9: ↑ jmp    282
> >>>>
> >>>>
> >>>> Given the patch at hand I was expecting lost IPIs, but I can't quite see
> >>>> anything getting lost.
> >>>
> >>> I have no idea how that patch should be related to IPI and smp function
> >>> calls. It's changing the way how regular device interrupts and their
> >>> spurious counterpart are handled and not the way how IPIs are
> >>> handled. They are handled by direct vectors and do not go through
> >>> do_IRQ() at all.
> >>>
> >>> Aside of that the commit just changes the way how the interrupt vector
> >>> of a regular device interrupt is stored and conveyed. The extra read and
> >>> write on the cache hot stack is hardly related to anything IPI.
> >>
> >> I am as puzzled as you are, but the bisect is very clear: 79b9c183021e
> >> works fast and 633260fa1 (as well as mainline) shows the weird behavior
> >> above.
> >>
> >> It gets even better. This small (demonstrative only, mangled) patch on
> >> top of 633260fa1 also resolves the performance issue:
> >>
> >> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> >> index c766936..7e91e9a 100644
> >> --- a/arch/x86/kernel/irq.c
> >> +++ b/arch/x86/kernel/irq.c
> >> @@ -239,6 +239,7 @@ __visible void __irq_entry do_IRQ(struct pt_regs
> >> *regs, unsigned long vector)
> >>           * lower 8 bits.
> >>           */
> >>          vector &= 0xFF;
> >> +       regs->orig_ax = ~vector;
> >>
> >>          /* entering_irq() tells RCU that we're not quiescent.  Check it. */
> >>          RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up RCU");
> >>
> >>
> >> To me that sounds like some irq exit code somewhere must still be
> >> looking at orig_ax to decide on something - and that something is wrong
> >> now that we removed the negation of the vector. It also seems to have an
> >> impact on remote function calls.
> >>
> >> I'll have a deeper look tomorrow again if I can find any such place, but
> >> I wouldn't mind if anyone could point me into the right direction
> >> earlier :).
> >
> > How about this:
> >
> > void irq_complete_move(struct irq_cfg *cfg)
> > {
> >          __irq_complete_move(cfg, ~get_irq_regs()->orig_ax);
> > }
> >
> > in arch/x86/kernel/apic/vector.c.
> >
>
> Thanks a lot, I stumbled over the same thing just after I sent the email
> as well and had been trying to see if I can quickly patch it up before I
> fall asleep :).
>
> The code path above is used by the APIC vector move (irqbalance) logic,
> which explains why not everyone was seeing issues.
>
> So with 633260fa1 applied, we never get out of moving state for our IRQ
> because orig_ax is always -1. That means we send an IPI to the cleanup
> vector on every single device interrupt, completely occupying the poor
> CPU that we moved the IRQ from.
>
> I've confirmed that the patch below fixes the issue and will send a
> proper, more complete patch on top of mainline with fancy description
> and stable tag tomorrow.
>
>
> Alex
>
>
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index e7434cd..a474e6e 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -734,7 +734,6 @@ SYM_CODE_START_LOCAL(common_spurious)
>         call    interrupt_entry
>         UNWIND_HINT_REGS indirect=1
>         movq    ORIG_RAX(%rdi), %rsi            /* get vector from stack */
> -       movq    $-1, ORIG_RAX(%rdi)             /* no syscall to restart */
>         call    smp_spurious_interrupt          /* rdi points to pt_regs */
>         jmp     ret_from_intr
>   SYM_CODE_END(common_spurious)
> @@ -746,7 +745,6 @@ SYM_CODE_START_LOCAL(common_interrupt)
>         call    interrupt_entry
>         UNWIND_HINT_REGS indirect=1
>         movq    ORIG_RAX(%rdi), %rsi            /* get vector from stack */
> -       movq    $-1, ORIG_RAX(%rdi)             /* no syscall to restart */
>         call    do_IRQ                          /* rdi points to pt_regs */
>         /* 0(%rsp): old RSP */
>   ret_from_intr:
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index 67768e5443..5b6f74e 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -934,7 +934,7 @@ static void __irq_complete_move(struct irq_cfg *cfg,
> unsigned vector)
>
>   void irq_complete_move(struct irq_cfg *cfg)
>   {
> -       __irq_complete_move(cfg, ~get_irq_regs()->orig_ax);
> +       __irq_complete_move(cfg, get_irq_regs()->orig_ax);
>   }

I think you need to also truncate the vector to 8-bits, since it now
gets sign-extended when pushed into the orig_ax slot.

--
Brian Gerst