[PATCH 0/3] Use NMI to stop cpus

[PATCH 0/3] Use NMI to stop cpus

[Don Zickus]

This patch series attempts to use NMI to stop cpus instead of the
REBOOT_VECTOR irq.  The idea is to have a more robust and reliable
way to stop cpus when serializing things like the panic path.

More details in the changelogs.

Don Zickus (3):
  x86, reboot:  Use NMI instead of REBOOT_VECTOR to stop cpus
  x86, NMI: Add NMI IPI selftest
  x86, NMI: knob to disable using NMI IPIs to stop cpus

 Documentation/kernel-parameters.txt |    4 ++
 arch/x86/Kconfig.debug              |   12 ++++++
 arch/x86/include/asm/smp.h          |    1 +
 arch/x86/kernel/smp.c               |   71 ++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/smpboot.c           |   57 ++++++++++++++++++++++++++++
 5 files changed, 143 insertions(+), 2 deletions(-)

-- 
1.7.6.4

[PATCH 1/3] x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus

[Don Zickus]

A recent discussion started talking about the locking on the pstore fs
and how it relates to the kmsg infrastructure.  We noticed it was possible
for userspace to r/w to the pstore fs (grabbing the locks in the process)
and block the panic path from r/w to the same fs.

The reason was the cpu with the lock could be doing work while the crashing
cpu is panic'ing.  Busting those spinlocks might cause those cpus to step
on each other's data.  Fine, fair enough.

It was suggested it would be nice to serialize the panic path (ie stop
the other cpus) and have only one cpu running.  This would allow us to
bust the spinlocks and not worry about another cpu stepping on the data.

Of course, smp_send_stop() does this in the panic case.  kmsg_dump() would
have to be moved to be called after it.  Easy enough.

The only problem is on x86 the smp_send_stop() function calls the
REBOOT_VECTOR.  Any cpu with irqs disabled (which pstore and its backend
ERST would do), block this IPI and thus do not stop.  This makes it
difficult to reliably log data to the pstore fs.

The patch below switches from the REBOOT_VECTOR to NMI (and mimics what
kdump does).  Switching to NMI allows us to deliver the IPI when irqs are
disabled, increasing the reliability of this function.

However, Andi carefully noted that on some machines this approach does not
work because of broken BIOSes or whatever.

To help accomodate this, the next couple of patches will run a selftest and
provide a knob to disable.

V2:
  uses atomic ops to serialize the cpu that shuts everyone down

Signed-off-by: Don Zickus <dzickus@redhat.com>
---

[note] this patch sits on top of another NMI infrastructure change I have
submitted, so the nmi registeration might not apply cleanly without that patch.
---
 arch/x86/kernel/smp.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 013e7eb..7bdbf6a 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -28,6 +28,7 @@
 #include <asm/mmu_context.h>
 #include <asm/proto.h>
 #include <asm/apic.h>
+#include <asm/nmi.h>
 /*
  *	Some notes on x86 processor bugs affecting SMP operation:
  *
@@ -147,6 +148,59 @@ 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 */
+	if (raw_smp_processor_id() == atomic_read(&stopping_cpu))
+		return NMI_HANDLED;
+
+	stop_this_cpu(NULL);
+
+	return NMI_HANDLED;
+}
+
+static void native_nmi_stop_other_cpus(int wait)
+{
+	unsigned long flags;
+	unsigned long timeout;
+
+	if (reboot_force)
+		return;
+
+	/*
+	 * Use an own vector here because smp_call_function
+	 * does lots of things not suitable in a panic situation.
+	 */
+	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"))
+			return;		/* return what? */
+
+		/* sync above data before sending NMI */
+		wmb();
+
+		apic->send_IPI_allbutself(NMI_VECTOR);
+
+		/*
+		 * Don't wait longer than a second if the caller
+		 * didn't ask us to wait.
+		 */
+		timeout = USEC_PER_SEC;
+		while (num_online_cpus() > 1 && (wait || timeout--))
+			udelay(1);
+	}
+
+	local_irq_save(flags);
+	disable_local_APIC();
+	local_irq_restore(flags);
+}
+
 /*
  * this function calls the 'stop' function on all other CPUs in the system.
  */
@@ -159,7 +213,7 @@ asmlinkage void smp_reboot_interrupt(void)
 	irq_exit();
 }
 
-static void native_stop_other_cpus(int wait)
+static void native_irq_stop_other_cpus(int wait)
 {
 	unsigned long flags;
 	unsigned long timeout;
@@ -229,7 +283,7 @@ struct smp_ops smp_ops = {
 	.smp_prepare_cpus	= native_smp_prepare_cpus,
 	.smp_cpus_done		= native_smp_cpus_done,
 
-	.stop_other_cpus	= native_stop_other_cpus,
+	.stop_other_cpus	= native_nmi_stop_other_cpus,
 	.smp_send_reschedule	= native_smp_send_reschedule,
 
 	.cpu_up			= native_cpu_up,
-- 
1.7.6.4

[PATCH 2/3] x86, NMI: Add NMI IPI selftest

[Don Zickus]

The previous patch modified the stop cpus path to use NMI instead of IRQ
as the way to communicate to the other cpus to shutdown.  There were some
concerns that various machines may have problems with using an NMI IPI.

This patch creates a selftest to check if NMI IPI is working at boot.
The idea is to help catch any issues before the machine panics and we
learn the hard way. :-)

If the selftest fails, the code tries to fall back to the original IRQ
method.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/Kconfig.debug     |   12 +++++++++
 arch/x86/include/asm/smp.h |    1 +
 arch/x86/kernel/smp.c      |    5 ++++
 arch/x86/kernel/smpboot.c  |   57 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index c0f8a5c..18261c2 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -284,4 +284,16 @@ config DEBUG_STRICT_USER_COPY_CHECKS
 
 	  If unsure, or if you run an older (pre 4.4) gcc, say N.
 
+config DEBUG_NMI_IPI_SELFTEST
+	bool "NMI IPI Selftest"
+	depends on DEBUG_KERNEL
+	---help---
+	  Enabling this option turns on a quick NMI IPI selftest to
+	  verify that sending an NMI this way works for the panic and
+	  kdump paths.
+
+	  This might help diagnose strange hangs in those paths.
+
+	  If unsure, say N.
+
 endmenu
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 73b11bc..916f89d 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -149,6 +149,7 @@ static inline void arch_send_call_function_ipi_mask(const struct cpumask *mask)
 }
 
 void cpu_disable_common(void);
+void native_smp_disable_nmi_ipi(void);
 void native_smp_prepare_boot_cpu(void);
 void native_smp_prepare_cpus(unsigned int max_cpus);
 void native_smp_cpus_done(unsigned int max_cpus);
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 7bdbf6a..f54e07a 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -247,6 +247,11 @@ static void native_irq_stop_other_cpus(int wait)
 	local_irq_restore(flags);
 }
 
+void native_smp_disable_nmi_ipi(void)
+{
+	smp_ops.stop_other_cpus = native_irq_stop_other_cpus;
+}
+
 /*
  * Reschedule call back.
  */
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 9f548cb..dcb889f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -71,6 +71,7 @@
 
 #include <asm/smpboot_hooks.h>
 #include <asm/i8259.h>
+#include <asm/nmi.h>
 
 /* State of each CPU */
 DEFINE_PER_CPU(int, cpu_state) = { 0 };
@@ -415,6 +416,57 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
 		return cpu_llc_shared_mask(cpu);
 }
 
+#ifdef CONFIG_DEBUG_NMI_IPI_SELFTEST
+/* check to see if NMI IPIs work on this machine */
+static DECLARE_BITMAP(nmi_ipi_mask, NR_CPUS) __read_mostly;
+
+static int check_nmi_ipi_callback(unsigned int val, struct pt_regs *regs)
+{
+	int cpu = raw_smp_processor_id();
+
+	if (cpumask_test_cpu(cpu, to_cpumask(nmi_ipi_mask))) {
+		cpumask_clear_cpu(cpu, to_cpumask(nmi_ipi_mask));
+		return NMI_HANDLED;
+	}
+
+	return NMI_DONE;
+}
+
+static bool check_nmi_ipi(void)
+{
+	unsigned long timeout;
+
+	if (num_online_cpus() > 1) {
+		cpumask_copy(to_cpumask(nmi_ipi_mask), cpu_online_mask);
+		cpumask_clear_cpu(smp_processor_id(), to_cpumask(nmi_ipi_mask));
+
+		if (register_nmi_handler(NMI_LOCAL, check_nmi_ipi_callback,
+					 NMI_FLAG_FIRST, "check_nmi_ipi"))
+			return false;
+
+		/* sync above data before sending NMI */
+		wmb();
+
+		apic->send_IPI_allbutself(NMI_VECTOR);
+
+		/* Don't wait longer than a second */
+		timeout = USEC_PER_SEC;
+		while (!cpumask_empty(to_cpumask(nmi_ipi_mask)) && timeout--)
+			udelay(1);
+
+		/* What happens if we timeout, do we still unregister?? */
+		unregister_nmi_handler(NMI_LOCAL, "check_nmi_ipi");
+
+		if (!timeout) {
+			pr_warn("CPU does not support NMI IPIs\n");
+			return false;
+		}
+	}
+	pr_info("NMI IPIs selftest passed\n");
+	return true;
+}
+#endif
+
 static void impress_friends(void)
 {
 	int cpu;
@@ -1142,6 +1194,11 @@ void __init native_smp_cpus_done(unsigned int max_cpus)
 {
 	pr_debug("Boot done.\n");
 
+#ifdef CONFIG_DEBUG_NMI_IPI_SELFTEST
+	/* if NMI IPI doesn't work, fallback to old irq method for panic */
+	if (check_nmi_ipi())
+		native_smp_disable_nmi_ipi();
+#endif
 	impress_friends();
 #ifdef CONFIG_X86_IO_APIC
 	setup_ioapic_dest();
-- 
1.7.6.4

[PATCH 3/3] x86, NMI: knob to disable using NMI IPIs to stop cpus

[Don Zickus]

Some machines may exhibit problems using the NMI to stop other cpus.
This knob just allows one to revert back to the original behaviour to help
diagnose the problem.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 Documentation/kernel-parameters.txt |    4 ++++
 arch/x86/kernel/smp.c               |    8 ++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 6ca1f5c..dab6439 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1761,6 +1761,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	nomfgpt		[X86-32] Disable Multi-Function General Purpose
 			Timer usage (for AMD Geode machines).
 
+	nonmi_ipi	[X86] Disable using NMI IPIs during panic/reboot to
+			shutdown the other cpus.  Instead use the REBOOT_VECTOR
+			irq.
+
 	nopat		[X86] Disable PAT (page attribute table extension of
 			pagetables) support.
 
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index f54e07a..cc8026a 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -283,6 +283,14 @@ void smp_call_function_single_interrupt(struct pt_regs *regs)
 	irq_exit();
 }
 
+int __init nonmi_ipi_setup(char *str)
+{
+        native_smp_disable_nmi_ipi();
+        return 1;
+}
+
+__setup("nonmi_ipi", nonmi_ipi_setup);
+
 struct smp_ops smp_ops = {
 	.smp_prepare_boot_cpu	= native_smp_prepare_boot_cpu,
 	.smp_prepare_cpus	= native_smp_prepare_cpus,
-- 
1.7.6.4

Re: [PATCH 1/3] x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus

[Chen Gong]

于 2011/10/11 23:24, Don Zickus 写道:
> A recent discussion started talking about the locking on the pstore fs
> and how it relates to the kmsg infrastructure.  We noticed it was possible
> for userspace to r/w to the pstore fs (grabbing the locks in the process)
> and block the panic path from r/w to the same fs.
>
> The reason was the cpu with the lock could be doing work while the crashing
> cpu is panic'ing.  Busting those spinlocks might cause those cpus to step
> on each other's data.  Fine, fair enough.
>
> It was suggested it would be nice to serialize the panic path (ie stop
> the other cpus) and have only one cpu running.  This would allow us to
> bust the spinlocks and not worry about another cpu stepping on the data.
>
> Of course, smp_send_stop() does this in the panic case.  kmsg_dump() would
> have to be moved to be called after it.  Easy enough.
>
> The only problem is on x86 the smp_send_stop() function calls the
> REBOOT_VECTOR.  Any cpu with irqs disabled (which pstore and its backend
> ERST would do), block this IPI and thus do not stop.  This makes it
> difficult to reliably log data to the pstore fs.
>
> The patch below switches from the REBOOT_VECTOR to NMI (and mimics what
> kdump does).  Switching to NMI allows us to deliver the IPI when irqs are
> disabled, increasing the reliability of this function.
>
> However, Andi carefully noted that on some machines this approach does not
> work because of broken BIOSes or whatever.
>
> To help accomodate this, the next couple of patches will run a selftest and
> provide a knob to disable.
>
> V2:
>    uses atomic ops to serialize the cpu that shuts everyone down
>
> Signed-off-by: Don Zickus<dzickus@redhat.com>
> ---
>
> [note] this patch sits on top of another NMI infrastructure change I have
> submitted, so the nmi registeration might not apply cleanly without that patch.
> ---
>   arch/x86/kernel/smp.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 013e7eb..7bdbf6a 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -28,6 +28,7 @@
>   #include<asm/mmu_context.h>
>   #include<asm/proto.h>
>   #include<asm/apic.h>
> +#include<asm/nmi.h>
>   /*
>    *	Some notes on x86 processor bugs affecting SMP operation:
>    *
> @@ -147,6 +148,59 @@ 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 */
> +	if (raw_smp_processor_id() == atomic_read(&stopping_cpu))
> +		return NMI_HANDLED;
> +
> +	stop_this_cpu(NULL);
> +
> +	return NMI_HANDLED;
> +}
> +
> +static void native_nmi_stop_other_cpus(int wait)
> +{
> +	unsigned long flags;
> +	unsigned long timeout;
> +
> +	if (reboot_force)
> +		return;
> +
> +	/*
> +	 * Use an own vector here because smp_call_function
> +	 * does lots of things not suitable in a panic situation.
> +	 */
> +	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"))
> +			return;		/* return what? */
> +
> +		/* sync above data before sending NMI */
> +		wmb();
> +
> +		apic->send_IPI_allbutself(NMI_VECTOR);
> +
> +		/*
> +		 * Don't wait longer than a second if the caller
> +		 * didn't ask us to wait.
> +		 */
> +		timeout = USEC_PER_SEC;
> +		while (num_online_cpus()>  1&&  (wait || timeout--))
> +			udelay(1);

In this patch and next patch, how about using the same logic in commit 74d91e3c6

Re: [PATCH 2/3] x86, NMI: Add NMI IPI selftest

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> The previous patch modified the stop cpus path to use NMI instead of IRQ
> as the way to communicate to the other cpus to shutdown.  There were some
> concerns that various machines may have problems with using an NMI IPI.
> 
> This patch creates a selftest to check if NMI IPI is working at boot.
> The idea is to help catch any issues before the machine panics and we
> learn the hard way. :-)
> 
> If the selftest fails, the code tries to fall back to the original IRQ
> method.
> 
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
>  arch/x86/Kconfig.debug     |   12 +++++++++
>  arch/x86/include/asm/smp.h |    1 +
>  arch/x86/kernel/smp.c      |    5 ++++
>  arch/x86/kernel/smpboot.c  |   57 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index c0f8a5c..18261c2 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -284,4 +284,16 @@ config DEBUG_STRICT_USER_COPY_CHECKS
>  
>  	  If unsure, or if you run an older (pre 4.4) gcc, say N.
>  
> +config DEBUG_NMI_IPI_SELFTEST
> +	bool "NMI IPI Selftest"
> +	depends on DEBUG_KERNEL
> +	---help---
> +	  Enabling this option turns on a quick NMI IPI selftest to
> +	  verify that sending an NMI this way works for the panic and
> +	  kdump paths.
> +
> +	  This might help diagnose strange hangs in those paths.
> +
> +	  If unsure, say N.
> +
>  endmenu
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 73b11bc..916f89d 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -149,6 +149,7 @@ static inline void arch_send_call_function_ipi_mask(const struct cpumask *mask)
>  }
>  
>  void cpu_disable_common(void);
> +void native_smp_disable_nmi_ipi(void);
>  void native_smp_prepare_boot_cpu(void);
>  void native_smp_prepare_cpus(unsigned int max_cpus);
>  void native_smp_cpus_done(unsigned int max_cpus);
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 7bdbf6a..f54e07a 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -247,6 +247,11 @@ static void native_irq_stop_other_cpus(int wait)
>  	local_irq_restore(flags);
>  }
>  
> +void native_smp_disable_nmi_ipi(void)
> +{
> +	smp_ops.stop_other_cpus = native_irq_stop_other_cpus;
> +}
> +
>  /*
>   * Reschedule call back.
>   */
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 9f548cb..dcb889f 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -71,6 +71,7 @@
>  
>  #include <asm/smpboot_hooks.h>
>  #include <asm/i8259.h>
> +#include <asm/nmi.h>
>  
>  /* State of each CPU */
>  DEFINE_PER_CPU(int, cpu_state) = { 0 };
> @@ -415,6 +416,57 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
>  		return cpu_llc_shared_mask(cpu);
>  }
>  
> +#ifdef CONFIG_DEBUG_NMI_IPI_SELFTEST
> +/* check to see if NMI IPIs work on this machine */
> +static DECLARE_BITMAP(nmi_ipi_mask, NR_CPUS) __read_mostly;
> +
> +static int check_nmi_ipi_callback(unsigned int val, struct pt_regs *regs)
> +{
> +	int cpu = raw_smp_processor_id();
> +
> +	if (cpumask_test_cpu(cpu, to_cpumask(nmi_ipi_mask))) {
> +		cpumask_clear_cpu(cpu, to_cpumask(nmi_ipi_mask));
> +		return NMI_HANDLED;
> +	}
> +
> +	return NMI_DONE;
> +}
> +
> +static bool check_nmi_ipi(void)
> +{
> +	unsigned long timeout;
> +
> +	if (num_online_cpus() > 1) {
> +		cpumask_copy(to_cpumask(nmi_ipi_mask), cpu_online_mask);
> +		cpumask_clear_cpu(smp_processor_id(), to_cpumask(nmi_ipi_mask));
> +
> +		if (register_nmi_handler(NMI_LOCAL, check_nmi_ipi_callback,
> +					 NMI_FLAG_FIRST, "check_nmi_ipi"))
> +			return false;
> +
> +		/* sync above data before sending NMI */
> +		wmb();
> +
> +		apic->send_IPI_allbutself(NMI_VECTOR);
> +
> +		/* Don't wait longer than a second */
> +		timeout = USEC_PER_SEC;
> +		while (!cpumask_empty(to_cpumask(nmi_ipi_mask)) && timeout--)
> +			udelay(1);
> +
> +		/* What happens if we timeout, do we still unregister?? */
> +		unregister_nmi_handler(NMI_LOCAL, "check_nmi_ipi");
> +
> +		if (!timeout) {
> +			pr_warn("CPU does not support NMI IPIs\n");
> +			return false;
> +		}
> +	}
> +	pr_info("NMI IPIs selftest passed\n");
> +	return true;
> +}
> +#endif
> +
>  static void impress_friends(void)
>  {
>  	int cpu;
> @@ -1142,6 +1194,11 @@ void __init native_smp_cpus_done(unsigned int max_cpus)
>  {
>  	pr_debug("Boot done.\n");
>  
> +#ifdef CONFIG_DEBUG_NMI_IPI_SELFTEST
> +	/* if NMI IPI doesn't work, fallback to old irq method for panic */
> +	if (check_nmi_ipi())
> +		native_smp_disable_nmi_ipi();
> +#endif

Looks good in principle, but instead of the #ifdeffery i'd really 
suggest to create a separate (small) .c module and .h header file for 
this, to hide all those self-test details from the generic path. The 
callback in native_smp_cpus_done() callback can be a plain 
nmi_selftest_callback() call or such.

Thanks,

	Ingo

Re: [PATCH 3/3] x86, NMI: knob to disable using NMI IPIs to stop cpus

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> +int __init nonmi_ipi_setup(char *str)

can be static.

Looks fine otherwise.

Thanks,

	Ingo

Re: [PATCH 1/3] x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> +		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
> +					 NMI_FLAG_FIRST, "smp_stop"))
> +			return;		/* return what? */

That comment looks a bit odd.

Thanks,

	Ingo

Re: [PATCH 1/3] x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus

[Don Zickus]

On Wed, Oct 12, 2011 at 10:35:42AM +0800, Chen Gong wrote:
> 于 2011/10/11 23:24, Don Zickus 写道:
> >+
> >+		/* sync above data before sending NMI */
> >+		wmb();
> >+
> >+		apic->send_IPI_allbutself(NMI_VECTOR);
> >+
> >+		/*
> >+		 * Don't wait longer than a second if the caller
> >+		 * didn't ask us to wait.
> >+		 */
> >+		timeout = USEC_PER_SEC;
> >+		while (num_online_cpus()>  1&&  (wait || timeout--))
> >+			udelay(1);
> 
> In this patch and next patch, how about using the same logic in commit 74d91e3c6

I guess I don't understand why I would do that.  That commit doesn't seem
to have a way to break out of the while loop and it does not take into account
the 'wait' variable the virt folks needed.  The only thing that could be
usable seems to be the 'touch_nmi_watchdog', but in that case if the other
cpus haven't hit the NMI yet, I would be happy for other NMI sources to
trigger to, to help move things along. :-)

The above code snippet is what is currently there and that seems to work
well, so I didn't want to change to much when moving from the IRQ path to
the NMI path.

Perhaps I am missing something?

Cheers,
Don

Re: [PATCH 1/3] x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus

[Don Zickus]

On Wed, Oct 12, 2011 at 09:30:25AM +0200, Ingo Molnar wrote:
> 
> * Don Zickus <dzickus@redhat.com> wrote:
> 
> > +		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
> > +					 NMI_FLAG_FIRST, "smp_stop"))
> > +			return;		/* return what? */
> 
> That comment looks a bit odd.

Yeah, I copied it from the kdump code because it seemed relevant.  The
point was to express the paranoid concern, if we can't register the NMI
handler for whatever reason, what happens!?.  How do we explain to anyone
we failed to shut down the other cpus?

I can expand the comment to be more specific in the paranoia.  I just
wasn't sure the right way to handle that failure case.

Thanks for your comments in the other patches, I'll work on those.

Cheers,
Don

Re: [PATCH 1/3] x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> On Wed, Oct 12, 2011 at 09:30:25AM +0200, Ingo Molnar wrote:
> > 
> > * Don Zickus <dzickus@redhat.com> wrote:
> > 
> > > +		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
> > > +					 NMI_FLAG_FIRST, "smp_stop"))
> > > +			return;		/* return what? */
> > 
> > That comment looks a bit odd.
> 
> Yeah, I copied it from the kdump code because it seemed relevant.  
> The point was to express the paranoid concern, if we can't register 
> the NMI handler for whatever reason, what happens!?.  How do we 
> explain to anyone we failed to shut down the other cpus?
> 
> I can expand the comment to be more specific in the paranoia.  I 
> just wasn't sure the right way to handle that failure case.

Just add something like:

	/* Note: we ignore failures here */

if there's nothing intelligent possible.

Thanks,

	Ingo

Re: [PATCH 1/3] x86, reboot: Use NMI instead of REBOOT_VECTOR to stop cpus

[Chen Gong]

于 2011/10/12 20:51, Don Zickus 写道:
> On Wed, Oct 12, 2011 at 10:35:42AM +0800, Chen Gong wrote:
>> 于 2011/10/11 23:24, Don Zickus 写道:
>>> +
>>> +		/* sync above data before sending NMI */
>>> +		wmb();
>>> +
>>> +		apic->send_IPI_allbutself(NMI_VECTOR);
>>> +
>>> +		/*
>>> +		 * Don't wait longer than a second if the caller
>>> +		 * didn't ask us to wait.
>>> +		 */
>>> +		timeout = USEC_PER_SEC;
>>> +		while (num_online_cpus()>   1&&   (wait || timeout--))
>>> +			udelay(1);
>>
>> In this patch and next patch, how about using the same logic in commit 74d91e3c6
>
> I guess I don't understand why I would do that.  That commit doesn't seem
> to have a way to break out of the while loop and it does not take into account
> the 'wait' variable the virt folks needed.  The only thing that could be
> usable seems to be the 'touch_nmi_watchdog', but in that case if the other
> cpus haven't hit the NMI yet, I would be happy for other NMI sources to
> trigger to, to help move things along. :-)

Maybe two birds with on stone :-)

>
> The above code snippet is what is currently there and that seems to work
> well, so I didn't want to change to much when moving from the IRQ path to
> the NMI path.

Fine, if so, I have no objection.