lkml.org 
[lkml]   [2020]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2] x86/hotplug: Silence APIC only after all irq's are migrated
    Date
    Ashok,

    On Thu, Aug 20 2020 at 17:42, Ashok Raj wrote:
    > When offlining CPUs, fixup_irqs() migrates all interrupts away from the
    > outgoing CPU to an online CPU. It's always possible the device sent an
    > interrupt to the previous CPU destination. Pending interrupt bit in IRR in
    > LAPIC identifies such interrupts. apic_soft_disable() will not capture any
    > new interrupts in IRR. This causes interrupts from device to be lost during
    > CPU offline. The issue was found when explicitly setting MSI affinity to a
    > CPU and immediately offlining it. It was simple to recreate with a USB
    > ethernet device and doing I/O to it while the CPU is offlined. Lost
    > interrupts happen even when Interrupt Remapping is enabled.

    New lines exist for a reason. They help to structure information. For
    the content, please see below.

    > Current code does apic_soft_disable() before migrating interrupts.
    >
    > native_cpu_disable()
    > {
    > ...
    > apic_soft_disable();
    > cpu_disable_common();
    > --> fixup_irqs(); // Too late to capture anything in IRR.
    > }
    >
    > Just flipping the above call sequence seems to hit the IRR checks
    > and the lost interrupt is fixed for both legacy MSI and when
    > interrupt remapping is enabled.

    Seems to hit? Come on, we really want changelogs which are based on
    facts and not on assumptions.

    Aside of that, yes that's a really subtle one and thanks for tracking it
    down! For some reason I never looked at that ordering, but now that you
    stick it in front of me, it's pretty clear that this is the root cause.

    > /*
    > * Disable the local APIC. Otherwise IPI broadcasts will reach
    > * it. It still responds normally to INIT, NMI, SMI, and SIPI
    > - * messages.
    > + * messages. It's important to do apic_soft_disable() after
    > + * fixup_irqs(), because fixup_irqs() called from cpu_disable_common()
    > + * depends on IRR being set.

    That sentence does not make sense to me.

    > + .... After apic_soft_disable() CPU preserves
    > + * currently set IRR/ISR but new interrupts will not set IRR.

    I agree with the IRR part, but ISR is simply impossible to be set in
    this situation.

    > + * This causes interrupts sent to outgoing CPU before completion
    > + * of IRQ migration to be lost. Check SDM Vol 3 "10.4.7.2 Local
    > + * APIC State after It Has been Software Disabled" section for more
    > + * details.

    Please do not use the SDM chapter number of today. It's going to be a
    different one with the next version.

    Something like this perhaps?

    /*
    * Disable the local APIC. Otherwise IPI broadcasts will reach
    * it. It still responds normally to INIT, NMI, SMI, and SIPI
    * messages.
    *
    * Disabling the APIC must happen after cpu_disable_common()
    * which invokes fixup_irqs().
    *
    * Disabling the APIC preserves already set bits in IRR, but
    * an interrupt arriving after disabling the local APIC does not
    * set the corresponding IRR bit.
    *
    * fixup_irqs() scans IRR for set bits so it can raise a not
    * yet handled interrupt on the new destination CPU via an IPI
    * but obviously it can't do so for IRR bits which are not set.
    * IOW, interrupts arriving after disabling the local APIC will
    * be lost.
    */

    Hmm?

    The changelog wants to have a corresponding update.

    Thanks,

    tglx

    \
     
     \ /
      Last update: 2020-08-26 02:41    [W:3.171 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site