Re: [PATCH v6 2/3] CPU hotplug, stop-machine: Plug race-window that leads to "IPI-to-offline-CPU"

From: Frederic Weisbecker
Date: Fri May 23 2014 - 09:23:01 EST

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 <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
> ---
>
> 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?

> break;
> case MULTI_STOP_RUN:
> if (is_active)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/