On 05/23/2014 06:52 PM, Frederic Weisbecker wrote: > On Fri, May 23, 2014 at 03:42:20PM +0530, Srivatsa S. Bhat wrote: >> During CPU offline, stop-machine is used to take control over all the online >> CPUs (via the per-cpu stopper thread) and then run take_cpu_down() on the CPU >> that is to be taken offline. >> >> But stop-machine itself has several stages: _PREPARE, _DISABLE_IRQ, _RUN etc. >> The important thing to note here is that the _DISABLE_IRQ stage comes much >> later after starting stop-machine, and hence there is a large window where >> other CPUs can send IPIs to the CPU going offline. As a result, we can >> encounter a scenario as depicted below, which causes IPIs to be sent to the >> CPU going offline, and that CPU notices them *after* it has gone offline, >> triggering the "IPI-to-offline-CPU" warning from the smp-call-function code. >> >> >> CPU 1 CPU 2 >> (Online CPU) (CPU going offline) >> >> Enter _PREPARE stage Enter _PREPARE stage >> >> Enter _DISABLE_IRQ stage >> >> >> = >> Got a device interrupt, | Didn't notice the IPI >> and the interrupt handler | since interrupts were >> called smp_call_function() | disabled on this CPU. >> and sent an IPI to CPU 2. | >> = >> >> >> Enter _DISABLE_IRQ stage >> >> >> Enter _RUN stage Enter _RUN stage >> >> = >> Busy loop with interrupts | Invoke take_cpu_down() >> disabled. | and take CPU 2 offline >> = >> >> >> Enter _EXIT stage Enter _EXIT stage >> >> Re-enable interrupts Re-enable interrupts >> >> The pending IPI is noted >> immediately, but alas, >> the CPU is offline at >> this point. >> >> >> >> So, as we can observe from this scenario, the IPI was sent when CPU 2 was >> still online, and hence it was perfectly legal. But unfortunately it was >> noted only after CPU 2 went offline, resulting in the warning from the >> IPI handling code. In other words, the fault was not at the sender, but >> at the receiver side - and if we look closely, the real bug is in the >> stop-machine sequence itself. >> >> The problem here is that the CPU going offline disabled its local interrupts >> (by entering _DISABLE_IRQ phase) *before* the other CPUs. And that's the >> reason why it was not able to respond to the IPI before going offline. >> >> A simple solution to this problem is to ensure that the CPU going offline >> disables its interrupts only *after* the other CPUs do the same thing. >> To achieve this, split the _DISABLE_IRQ state into 2 parts: >> >> 1st part: MULTI_STOP_DISABLE_IRQ_INACTIVE, where only the non-active CPUs >> (i.e., the "other" CPUs) disable their interrupts. >> >> 2nd part: MULTI_STOP_DISABLE_IRQ_ACTIVE, where the active CPU (i.e., the >> CPU going offline) disables its interrupts. >> >> With this in place, the CPU going offline will always be the last one to >> disable interrupts. After this step, no further IPIs can be sent to the >> outgoing CPU, since all the other CPUs would be executing the stop-machine >> code with interrupts disabled. And by the time stop-machine ends, the CPU >> would have gone offline and disappeared from the cpu_online_mask, and hence >> future invocations of smp_call_function() and friends will automatically >> prune that CPU out. Thus, we can guarantee that no CPU will end up >> *inadvertently* sending IPIs to an offline CPU. >> >> Signed-off-by: Srivatsa S. Bhat <[email protected]> >> --- >> >> kernel/stop_machine.c | 39 ++++++++++++++++++++++++++++++++++----- >> 1 file changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c >> index 01fbae5..288f7fe 100644 >> --- a/kernel/stop_machine.c >> +++ b/kernel/stop_machine.c >> @@ -130,8 +130,10 @@ enum multi_stop_state { >> MULTI_STOP_NONE, >> /* Awaiting everyone to be scheduled. */ >> MULTI_STOP_PREPARE, >> - /* Disable interrupts. */ >> - MULTI_STOP_DISABLE_IRQ, >> + /* Disable interrupts on CPUs not in ->active_cpus mask. */ >> + MULTI_STOP_DISABLE_IRQ_INACTIVE, >> + /* Disable interrupts on CPUs in ->active_cpus mask. */ >> + MULTI_STOP_DISABLE_IRQ_ACTIVE, >> /* Run the function */ >> MULTI_STOP_RUN, >> /* Exit */ >> @@ -189,12 +191,39 @@ static int multi_cpu_stop(void *data) >> do { >> /* Chill out and ensure we re-read multi_stop_state. */ >> cpu_relax(); >> + >> + /* >> + * We use 2 separate stages to disable interrupts, namely >> + * _INACTIVE and _ACTIVE, to ensure that the inactive CPUs >> + * disable their interrupts first, followed by the active CPUs. >> + * >> + * This is done to avoid a race in the CPU offline path, which >> + * can lead to receiving IPIs on the outgoing CPU *after* it >> + * has gone offline. >> + * >> + * During CPU offline, we don't want the other CPUs to send >> + * IPIs to the active_cpu (the outgoing CPU) *after* it has >> + * disabled interrupts (because, then it will notice the IPIs >> + * only after it has gone offline). We can prevent this by >> + * making the other CPUs disable their interrupts first - that >> + * way, they will run the stop-machine code with interrupts >> + * disabled, and hence won't send IPIs after that point. >> + */ >> + >> if (msdata->state != curstate) { >> curstate = msdata->state; >> switch (curstate) { >> - case MULTI_STOP_DISABLE_IRQ: >> - local_irq_disable(); >> - hard_irq_disable(); >> + case MULTI_STOP_DISABLE_IRQ_INACTIVE: >> + if (!is_active) { >> + local_irq_disable(); >> + hard_irq_disable(); >> + } >> + break; >> + case MULTI_STOP_DISABLE_IRQ_ACTIVE: >> + if (is_active) { >> + local_irq_disable(); >> + hard_irq_disable(); >> + } > > Do we actually need that now that we are flushing the ipi queue on CPU dying? >
Yes, we do. Flushing the IPI queue is one thing - it guarantees that a CPU doesn't go offline without finishing its work. Not receiving IPIs after going offline is a different thing - it helps avoid warnings from the IPI handling code (although it will be harmless if the queue had been flushed earlier). So I think it is good to have both, so that we can keep CPU offline very clean - no pending work left around, as well as no possibility of (real or spurious) warnings. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
