Re: [PATCH 1/2] x86, reboot: revert stop_other_cpus to using IRQ with NMI fallback

[Don Zickus <dzickus@redhat.com>]

On Fri, Apr 27, 2012 at 12:42:51PM -0400, Seiji Aguchi wrote:
> Hi,
> 
> I tested this patch by applying to -tip tree on 4cpu machine.
> 
> In a case where one cpu panics while external interrupt is disable in other cpus, 
> WARN_ON in native_smp_send_reschedule() hits.
> The problem is the same as follows which happened in reboot case.
> 
> https://lkml.org/lkml/2012/4/5/308
> 
> I personally think that reasonable solution is just skipping WARN_ON in reboot and panic case
> By adding "PANIC" state in system_state.
> 
> This issue has not solved since it was initially reported in February.
> A band-aid patch is better than doing nothing.
> 
> What do you think?

I don't think Peter would let that patch in.  I just haven't had time to
explore the alternatives.  But that issue isn't really related to what the
attached patch is about.

Cheers,
Don

> 
> Seiji
> 
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Don Zickus
> > Sent: Thursday, March 29, 2012 4:24 PM
> > To: x86@kernel.org
> > Cc: LKML; Peter Zijlstra; Don Zickus
> > Subject: [PATCH 1/2] x86, reboot: revert stop_other_cpus to using IRQ with NMI fallback
> > 
> > For v3.3, I added code to use the NMI to stop other cpus in the panic case.  The idea was to make sure all cpus on the system were
> > definitely halted to help serialize the panic path to execute the rest of the code on a single cpu.
> > 
> > The main problem it was trying to solve was how to stop a cpu that was spinning with its irqs disabled.  A IPI irq would be stuck and
> > couldn't get in there, but an NMI could.
> > 
> > Things were great until we had another conversation about some pstore changes.  Because some of the backend pstore still uses
> > spinlocks to protect the device access, things could get ugly if a panic happened and we were stuck spinning on a lock.
> > 
> > Now with the NMI shutting down cpus, we could assume no other cpus were running and just bust the spin lock and proceed.
> > 
> > The counter argument was, well if you do that the backend could be in a screwed up state and you might not be able to save anything
> > as a result.
> > If we could have just given the cpu a little more time to finish things, we could have grabbed the spin lock cleanly and everything
> > would have been fine.
> > 
> > Well, how do give a cpu a 'little more time' in the panic case?  For the most part you can't without spinning on the lock and even in that
> > case, how long do you spin for?
> > 
> > So instead of making it ugly in the pstore code, I had the idea that most spin locks are held with irqs disabled, naturally blocking other
> > irqs until they are done.  We just need an irq to sit there and grab the cpu once the lock is released and irqs are re-enabled again.
> > 
> > I decided to modify stop_other_cpus to go back to the REBOOT_IRQ code and use that IPI irq as the blocking irq.  This code has been
> > working for a long time and will give up after one second.  To fix the original problem of what happens after one second if a cpu hasn't
> > accepted the IPI, I just added the NMI hammer to clobber the cpu.
> > 
> > The end result of this more complicated-looking-diff-than-it-really-is, is a patch that mostly reverts
> > 
> > 3603a25 x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus
> > 
> > and instead just sticks another if-case after the REBOOT_IRQ and checks to see if online_cpus are still > 1, and if so, clobber the rest of
> > the cpus with an NMI.
> > 
> > For the most part, the NMI piece will never get hit, thus behaving just like
> > pre-v3.3 code.  However, in those rare conditions, we have a fallback plan.
> > 
> > I still wrap the NMI check with the knob 'nonmi_ipi' in case someone still has issues with NMIs in the panic path.
> > 
> > I also reset the original names of the functions.
> > 
> > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > ---
> > 
> > There was some discussion on this earlier about how it was causing a scheduler WARN with the original NMI implementation of this
> > patch.  However, that is due to the way the cpu is being shutdown without communicating that information to the scheduler.
> > As a result during shutdown of the cpus the scheduler may reschedule a process onto a cpu that suddenly went away and spit out a
> > WARN().
> > 
> > That is a separate problem then what I am trying to solve here.  I hacked up patch to solve that problem but it doesn't seem to work
> > reliably yet so I haven't posted it yet.
> > 
> >  arch/x86/kernel/smp.c |  100 ++++++++++++++++++++++--------------------------
> >  1 files changed, 46 insertions(+), 54 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c index 66c74f4..48d2b7d 100644
> > --- a/arch/x86/kernel/smp.c
> > +++ b/arch/x86/kernel/smp.c
> > @@ -109,6 +109,9 @@
> >   *	about nothing of note with C stepping upwards.
> >   */
> > 
> > +static atomic_t stopping_cpu = ATOMIC_INIT(-1); static bool
> > +smp_no_nmi_ipi = false;
> > +
> >  /*
> >   * this function sends a 'reschedule' IPI to another CPU.
> >   * it goes straight through and wastes no time serializing @@ -149,8 +152,6 @@ void native_send_call_func_ipi(const struct cpumask
> > *mask)
> >  	free_cpumask_var(allbutself);
> >  }
> > 
> > -static atomic_t stopping_cpu = ATOMIC_INIT(-1);
> > -
> >  static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)  {
> >  	/* We are registered on stopping cpu too, avoid spurious NMI */ @@ -162,7 +163,19 @@ static int
> > smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
> >  	return NMI_HANDLED;
> >  }
> > 
> > -static void native_nmi_stop_other_cpus(int wait)
> > +/*
> > + * this function calls the 'stop' function on all other CPUs in the system.
> > + */
> > +
> > +asmlinkage void smp_reboot_interrupt(void) {
> > +	ack_APIC_irq();
> > +	irq_enter();
> > +	stop_this_cpu(NULL);
> > +	irq_exit();
> > +}
> > +
> > +static void native_stop_other_cpus(int wait)
> >  {
> >  	unsigned long flags;
> >  	unsigned long timeout;
> > @@ -174,20 +187,25 @@ static void native_nmi_stop_other_cpus(int wait)
> >  	 * Use an own vector here because smp_call_function
> >  	 * does lots of things not suitable in a panic situation.
> >  	 */
> > +
> > +	/*
> > +	 * We start by using the REBOOT_VECTOR irq.
> > +	 * The irq is treated as a sync point to allow critical
> > +	 * regions of code on other cpus to release their spin locks
> > +	 * and re-enable irqs.  Jumping straight to an NMI might
> > +	 * accidentally cause deadlocks with further shutdown/panic
> > +	 * code.  By syncing, we give the cpus up to one second to
> > +	 * finish their work before we force them off with the NMI.
> > +	 */
> >  	if (num_online_cpus() > 1) {
> >  		/* did someone beat us here? */
> >  		if (atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id()) != -1)
> >  			return;
> > 
> > -		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
> > -					 NMI_FLAG_FIRST, "smp_stop"))
> > -			/* Note: we ignore failures here */
> > -			return;
> > -
> > -		/* sync above data before sending NMI */
> > +		/* sync above data before sending IRQ */
> >  		wmb();
> > 
> > -		apic->send_IPI_allbutself(NMI_VECTOR);
> > +		apic->send_IPI_allbutself(REBOOT_VECTOR);
> > 
> >  		/*
> >  		 * Don't wait longer than a second if the caller @@ -197,63 +215,37 @@ static void native_nmi_stop_other_cpus(int
> > wait)
> >  		while (num_online_cpus() > 1 && (wait || timeout--))
> >  			udelay(1);
> >  	}
> > +
> > +	/* if the REBOOT_VECTOR didn't work, try with the NMI */
> > +	if ((num_online_cpus() > 1) && (!smp_no_nmi_ipi))  {
> > +		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
> > +					 NMI_FLAG_FIRST, "smp_stop"))
> > +			/* Note: we ignore failures here */
> > +			/* Hope the REBOOT_IRQ is good enough */
> > +			goto finish;
> > 
> > -	local_irq_save(flags);
> > -	disable_local_APIC();
> > -	local_irq_restore(flags);
> > -}
> > -
> > -/*
> > - * this function calls the 'stop' function on all other CPUs in the system.
> > - */
> > -
> > -asmlinkage void smp_reboot_interrupt(void) -{
> > -	ack_APIC_irq();
> > -	irq_enter();
> > -	stop_this_cpu(NULL);
> > -	irq_exit();
> > -}
> > -
> > -static void native_irq_stop_other_cpus(int wait) -{
> > -	unsigned long flags;
> > -	unsigned long timeout;
> > +		/* sync above data before sending IRQ */
> > +		wmb();
> > 
> > -	if (reboot_force)
> > -		return;
> > +		pr_emerg("Shutting down cpus with NMI\n");
> > 
> > -	/*
> > -	 * Use an own vector here because smp_call_function
> > -	 * does lots of things not suitable in a panic situation.
> > -	 * On most systems we could also use an NMI here,
> > -	 * but there are a few systems around where NMI
> > -	 * is problematic so stay with an non NMI for now
> > -	 * (this implies we cannot stop CPUs spinning with irq off
> > -	 * currently)
> > -	 */
> > -	if (num_online_cpus() > 1) {
> > -		apic->send_IPI_allbutself(REBOOT_VECTOR);
> > +		apic->send_IPI_allbutself(NMI_VECTOR);
> > 
> >  		/*
> > -		 * Don't wait longer than a second if the caller
> > +		 * Don't wait longer than a 10 ms if the caller
> >  		 * didn't ask us to wait.
> >  		 */
> > -		timeout = USEC_PER_SEC;
> > +		timeout = USEC_PER_MSEC * 10;
> >  		while (num_online_cpus() > 1 && (wait || timeout--))
> >  			udelay(1);
> >  	}
> > 
> > +finish:
> >  	local_irq_save(flags);
> >  	disable_local_APIC();
> >  	local_irq_restore(flags);
> >  }
> > 
> > -static void native_smp_disable_nmi_ipi(void) -{
> > -	smp_ops.stop_other_cpus = native_irq_stop_other_cpus;
> > -}
> > -
> >  /*
> >   * Reschedule call back.
> >   */
> > @@ -287,8 +279,8 @@ void smp_call_function_single_interrupt(struct pt_regs *regs)
> > 
> >  static int __init nonmi_ipi_setup(char *str)  {
> > -        native_smp_disable_nmi_ipi();
> > -        return 1;
> > +	smp_no_nmi_ipi = true;
> > +	return 1;
> >  }
> > 
> >  __setup("nonmi_ipi", nonmi_ipi_setup);
> > @@ -298,7 +290,7 @@ struct smp_ops smp_ops = {
> >  	.smp_prepare_cpus	= native_smp_prepare_cpus,
> >  	.smp_cpus_done		= native_smp_cpus_done,
> > 
> > -	.stop_other_cpus	= native_nmi_stop_other_cpus,
> > +	.stop_other_cpus	= native_stop_other_cpus,
> >  	.smp_send_reschedule	= native_smp_send_reschedule,
> > 
> >  	.cpu_up			= native_cpu_up,
> > --
> > 1.7.7.6
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More
> > majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/