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

[Frederic Weisbecker <fweisbec@gmail.com>]

On Fri, May 23, 2014 at 08:15:35PM +0530, Srivatsa S. Bhat wrote:
> 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 <srivatsa.bhat@linux.vnet.ibm.com>
> >> ---
> >>
> >>  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).

I'm confused. Perhaps I don't understand well how things mix up. How does it avoid the warning.
Isn't there still a risk that some IPI don't fire due to hardware latency.

I mean either we do:

    local_irq_enable()
    wait_for_pending_ipi()
    local_irq_disable()

Or we do

    hotplug_cpu_down {
        flush_ipi()
    }

But something in between looks broken:

    local_irq_disable()
    local_irq_enable()

    flush_ipi()


> 
> 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.

Ah may be what you want to avoid is this:

    CPU 0               CPU 1
    -------------------------

    send IPI to 1

                        flush_ipi()
                        set_cpu_offline()
                        get_ipi()
                        //get late IPI but queue is flushed already
                        smp_single_function_interrupt() {
                               WARN()

Yeah but still, your patch doesn't deal with late hardware IPI.
How about we move the warning to the IPI callback iterator:

-       WARN_ON_ONCE(cpu_is_offline())

        llist_for_each(...) {
+            WARN_ON_ONCE(cpu_is_offline())
             csd->func()
        }

Since what matters is that all functions are executed before going offline.