[patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[Ingo Molnar]

Subject: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()
From: Ingo Molnar <mingo@elte.hu>

during kernel bootup, a new T60 laptop (CoreDuo, 32-bit) hangs about 
10%-20% of the time in acpi_init():

 Calling initcall 0xc055ce1a: topology_init+0x0/0x2f()
 Calling initcall 0xc055d75e: mtrr_init_finialize+0x0/0x2c()
 Calling initcall 0xc05664f3: param_sysfs_init+0x0/0x175()
 Calling initcall 0xc014cb65: pm_sysrq_init+0x0/0x17()
 Calling initcall 0xc0569f99: init_bio+0x0/0xf4()
 Calling initcall 0xc056b865: genhd_device_init+0x0/0x50()
 Calling initcall 0xc056c4bd: fbmem_init+0x0/0x87()
 Calling initcall 0xc056dd74: acpi_init+0x0/0x1ee()

it's a hard hang that not even an NMI could punch through! 
Frustratingly, adding printks or function tracing to the ACPI code made 
the hangs go away ...

after some time an additional detail emerged: disabling the NMI watchdog 
made these occasional hangs go away.

So i spent the better part of today trying to debug this and trying out 
various theories when i finally found the likely reason for the hang: if 
acpi_ns_initialize_devices() executes an _INI AML method and an NMI 
happens to hit that AML execution in the wrong moment, the machine would 
hang.  (my theory is that this must be some sort of chipset setup method 
doing stores to chipset mmio registers?)

Unfortunately given the characteristics of the hang it was sheer 
impossible to figure out which of the numerous AML methods is impacted 
by this problem.

as a workaround i wrote an interface to disable chipset-based NMIs while 
executing _INI sections - and indeed this fixed the hang. I did a 
boot-loop of 100 separate reboots and none hung - while without the 
patch it would hang every 5-10 attempts. Out of caution i did not touch 
the nmi_watchdog=2 case (it's not related to the chipset anyway and 
didnt hang).

I implemented this for both x86_64 and i686, tested the i686 laptop both 
with nmi_watchdog=1 [which triggered the hangs] and nmi_watchdog=2, and 
tested an Athlon64 box with the 64-bit kernel as well. Everything builds 
and works with the patch applied.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/i386/kernel/nmi.c          |   28 ++++++++++++++++++++++++++++
 arch/x86_64/kernel/nmi.c        |   27 +++++++++++++++++++++++++++
 drivers/acpi/namespace/nsinit.c |    9 +++++++++
 include/linux/nmi.h             |    9 ++++++++-
 4 files changed, 72 insertions(+), 1 deletion(-)

Index: linux/arch/i386/kernel/nmi.c
===================================================================
--- linux.orig/arch/i386/kernel/nmi.c
+++ linux/arch/i386/kernel/nmi.c
@@ -376,6 +376,34 @@ void enable_timer_nmi_watchdog(void)
 	}
 }
 
+static void __acpi_nmi_disable(void *__unused)
+{
+	apic_write_around(APIC_LVT0, APIC_DM_NMI | APIC_LVT_MASKED);
+}
+
+/*
+ * Disable timer based NMIs on all CPUs:
+ */
+void acpi_nmi_disable(void)
+{
+	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
+		on_each_cpu(__acpi_nmi_disable, NULL, 0, 1);
+}
+
+static void __acpi_nmi_enable(void *__unused)
+{
+	apic_write_around(APIC_LVT0, APIC_DM_NMI);
+}
+
+/*
+ * Enable timer based NMIs on all CPUs:
+ */
+void acpi_nmi_enable(void)
+{
+	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
+		on_each_cpu(__acpi_nmi_enable, NULL, 0, 1);
+}
+
 #ifdef CONFIG_PM
 
 static int nmi_pm_active; /* nmi_active before suspend */
Index: linux/arch/x86_64/kernel/nmi.c
===================================================================
--- linux.orig/arch/x86_64/kernel/nmi.c
+++ linux/arch/x86_64/kernel/nmi.c
@@ -360,6 +360,33 @@ void enable_timer_nmi_watchdog(void)
 	}
 }
 
+static void __acpi_nmi_disable(void *__unused)
+{
+	apic_write(APIC_LVT0, APIC_DM_NMI | APIC_LVT_MASKED);
+}
+
+/*
+ * Disable timer based NMIs on all CPUs:
+ */
+void acpi_nmi_disable(void)
+{
+	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
+		on_each_cpu(__acpi_nmi_disable, NULL, 0, 1);
+}
+
+static void __acpi_nmi_enable(void *__unused)
+{
+	apic_write(APIC_LVT0, APIC_DM_NMI);
+}
+
+/*
+ * Enable timer based NMIs on all CPUs:
+ */
+void acpi_nmi_enable(void)
+{
+	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
+		on_each_cpu(__acpi_nmi_enable, NULL, 0, 1);
+}
 #ifdef CONFIG_PM
 
 static int nmi_pm_active; /* nmi_active before suspend */
Index: linux/drivers/acpi/namespace/nsinit.c
===================================================================
--- linux.orig/drivers/acpi/namespace/nsinit.c
+++ linux/drivers/acpi/namespace/nsinit.c
@@ -45,6 +45,7 @@
 #include <acpi/acnamesp.h>
 #include <acpi/acdispat.h>
 #include <acpi/acinterp.h>
+#include <linux/nmi.h>
 
 #define _COMPONENT          ACPI_NAMESPACE
 ACPI_MODULE_NAME("nsinit")
@@ -537,7 +538,15 @@ acpi_ns_init_one_device(acpi_handle obj_
 	info->parameter_type = ACPI_PARAM_ARGS;
 	info->flags = ACPI_IGNORE_RETURN_VALUE;
 
+	/*
+	 * Some hardware relies on this being executed as atomically
+	 * as possible (without an NMI being received in the middle of
+	 * this) - so disable NMIs and initialize the device:
+	 */
+	acpi_nmi_disable();
 	status = acpi_ns_evaluate(info);
+	acpi_nmi_enable();
+
 	if (ACPI_SUCCESS(status)) {
 		walk_info->num_INI++;
 
Index: linux/include/linux/nmi.h
===================================================================
--- linux.orig/include/linux/nmi.h
+++ linux/include/linux/nmi.h
@@ -16,8 +16,15 @@
  */
 #ifdef ARCH_HAS_NMI_WATCHDOG
 extern void touch_nmi_watchdog(void);
+extern void acpi_nmi_disable(void);
+extern void acpi_nmi_enable(void);
 #else
-# define touch_nmi_watchdog() touch_softlockup_watchdog()
+static inline void touch_nmi_watchdog(void)
+{
+	touch_softlockup_watchdog();
+}
+static inline void acpi_nmi_disable(void) { }
+static inline void acpi_nmi_enable(void) { }
 #endif
 
 #endif

Re: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[Len Brown]

On Wednesday 06 December 2006 17:30, Ingo Molnar wrote:
> Subject: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()
> From: Ingo Molnar <mingo@elte.hu>
> 
> during kernel bootup, a new T60 laptop (CoreDuo, 32-bit) hangs about 
> 10%-20% of the time in acpi_init():
> 
>  Calling initcall 0xc055ce1a: topology_init+0x0/0x2f()
>  Calling initcall 0xc055d75e: mtrr_init_finialize+0x0/0x2c()
>  Calling initcall 0xc05664f3: param_sysfs_init+0x0/0x175()
>  Calling initcall 0xc014cb65: pm_sysrq_init+0x0/0x17()
>  Calling initcall 0xc0569f99: init_bio+0x0/0xf4()
>  Calling initcall 0xc056b865: genhd_device_init+0x0/0x50()
>  Calling initcall 0xc056c4bd: fbmem_init+0x0/0x87()
>  Calling initcall 0xc056dd74: acpi_init+0x0/0x1ee()
> 
> it's a hard hang that not even an NMI could punch through! 
> Frustratingly, adding printks or function tracing to the ACPI code made 
> the hangs go away ...
> 
> after some time an additional detail emerged: disabling the NMI watchdog 
> made these occasional hangs go away.
> 
> So i spent the better part of today trying to debug this and trying out 
> various theories when i finally found the likely reason for the hang: if 
> acpi_ns_initialize_devices() executes an _INI AML method and an NMI 
> happens to hit that AML execution in the wrong moment, the machine would 
> hang.  (my theory is that this must be some sort of chipset setup method 
> doing stores to chipset mmio registers?)
> 
> Unfortunately given the characteristics of the hang it was sheer 
> impossible to figure out which of the numerous AML methods is impacted 
> by this problem.
> 
> as a workaround i wrote an interface to disable chipset-based NMIs while 
> executing _INI sections - and indeed this fixed the hang. I did a 
> boot-loop of 100 separate reboots and none hung - while without the 
> patch it would hang every 5-10 attempts. Out of caution i did not touch 
> the nmi_watchdog=2 case (it's not related to the chipset anyway and 
> didnt hang).
> 
> I implemented this for both x86_64 and i686, tested the i686 laptop both 
> with nmi_watchdog=1 [which triggered the hangs] and nmi_watchdog=2, and 
> tested an Athlon64 box with the 64-bit kernel as well. Everything builds 
> and works with the patch applied.

Good work root-causing this failure!

Personally I have never been a big fan of having the NMI watchdog
running by default on all systems -- but Andi insists that it helps him
debug failures, so tick it does...

Clearly this laptop was validated with Windows, and Windows doesn't
have this problem -- suggesting that we may be somewhat out on a limb...

Some alternatives to consider...
a. don't enable NMI watchdog by default
b. enable NMI watchdog, but later during kernel boot
    (assumption here that the issue is only during _INI)
c. disable the NMI whenever the ACPI interpeter is running
   (who knows, maybe this isn't limited to the _INI case, but
    could cause a hang at some other time -- only the
    BIOS AML writers knows....)

thoughts?
-Len

> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/i386/kernel/nmi.c          |   28 ++++++++++++++++++++++++++++
>  arch/x86_64/kernel/nmi.c        |   27 +++++++++++++++++++++++++++
>  drivers/acpi/namespace/nsinit.c |    9 +++++++++
>  include/linux/nmi.h             |    9 ++++++++-
>  4 files changed, 72 insertions(+), 1 deletion(-)
> 
> Index: linux/arch/i386/kernel/nmi.c
> ===================================================================
> --- linux.orig/arch/i386/kernel/nmi.c
> +++ linux/arch/i386/kernel/nmi.c
> @@ -376,6 +376,34 @@ void enable_timer_nmi_watchdog(void)
>  	}
>  }
>  
> +static void __acpi_nmi_disable(void *__unused)
> +{
> +	apic_write_around(APIC_LVT0, APIC_DM_NMI | APIC_LVT_MASKED);
> +}
> +
> +/*
> + * Disable timer based NMIs on all CPUs:
> + */
> +void acpi_nmi_disable(void)
> +{
> +	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
> +		on_each_cpu(__acpi_nmi_disable, NULL, 0, 1);
> +}
> +
> +static void __acpi_nmi_enable(void *__unused)
> +{
> +	apic_write_around(APIC_LVT0, APIC_DM_NMI);
> +}
> +
> +/*
> + * Enable timer based NMIs on all CPUs:
> + */
> +void acpi_nmi_enable(void)
> +{
> +	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
> +		on_each_cpu(__acpi_nmi_enable, NULL, 0, 1);
> +}
> +
>  #ifdef CONFIG_PM
>  
>  static int nmi_pm_active; /* nmi_active before suspend */
> Index: linux/arch/x86_64/kernel/nmi.c
> ===================================================================
> --- linux.orig/arch/x86_64/kernel/nmi.c
> +++ linux/arch/x86_64/kernel/nmi.c
> @@ -360,6 +360,33 @@ void enable_timer_nmi_watchdog(void)
>  	}
>  }
>  
> +static void __acpi_nmi_disable(void *__unused)
> +{
> +	apic_write(APIC_LVT0, APIC_DM_NMI | APIC_LVT_MASKED);
> +}
> +
> +/*
> + * Disable timer based NMIs on all CPUs:
> + */
> +void acpi_nmi_disable(void)
> +{
> +	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
> +		on_each_cpu(__acpi_nmi_disable, NULL, 0, 1);
> +}
> +
> +static void __acpi_nmi_enable(void *__unused)
> +{
> +	apic_write(APIC_LVT0, APIC_DM_NMI);
> +}
> +
> +/*
> + * Enable timer based NMIs on all CPUs:
> + */
> +void acpi_nmi_enable(void)
> +{
> +	if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
> +		on_each_cpu(__acpi_nmi_enable, NULL, 0, 1);
> +}
>  #ifdef CONFIG_PM
>  
>  static int nmi_pm_active; /* nmi_active before suspend */
> Index: linux/drivers/acpi/namespace/nsinit.c
> ===================================================================
> --- linux.orig/drivers/acpi/namespace/nsinit.c
> +++ linux/drivers/acpi/namespace/nsinit.c
> @@ -45,6 +45,7 @@
>  #include <acpi/acnamesp.h>
>  #include <acpi/acdispat.h>
>  #include <acpi/acinterp.h>
> +#include <linux/nmi.h>
>  
>  #define _COMPONENT          ACPI_NAMESPACE
>  ACPI_MODULE_NAME("nsinit")
> @@ -537,7 +538,15 @@ acpi_ns_init_one_device(acpi_handle obj_
>  	info->parameter_type = ACPI_PARAM_ARGS;
>  	info->flags = ACPI_IGNORE_RETURN_VALUE;
>  
> +	/*
> +	 * Some hardware relies on this being executed as atomically
> +	 * as possible (without an NMI being received in the middle of
> +	 * this) - so disable NMIs and initialize the device:
> +	 */
> +	acpi_nmi_disable();
>  	status = acpi_ns_evaluate(info);
> +	acpi_nmi_enable();
> +
>  	if (ACPI_SUCCESS(status)) {
>  		walk_info->num_INI++;
>  
> Index: linux/include/linux/nmi.h
> ===================================================================
> --- linux.orig/include/linux/nmi.h
> +++ linux/include/linux/nmi.h
> @@ -16,8 +16,15 @@
>   */
>  #ifdef ARCH_HAS_NMI_WATCHDOG
>  extern void touch_nmi_watchdog(void);
> +extern void acpi_nmi_disable(void);
> +extern void acpi_nmi_enable(void);
>  #else
> -# define touch_nmi_watchdog() touch_softlockup_watchdog()
> +static inline void touch_nmi_watchdog(void)
> +{
> +	touch_softlockup_watchdog();
> +}
> +static inline void acpi_nmi_disable(void) { }
> +static inline void acpi_nmi_enable(void) { }
>  #endif
>  
>  #endif
> -
> 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/
> 

Re: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[Sergio Monteiro Basto]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

On Wed, 2006-12-06 at 23:30 +0100, Ingo Molnar wrote:
> Subject: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()
> From: Ingo Molnar <mingo@elte.hu>
Hi Ingo,
Just curiosity ,have we this patch on 2.6.19-1.rt6 ?

Thanks,
-- 
Sérgio M.B.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2166 bytes --]

Re: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[Karsten Wiese]

Am Donnerstag, 7. Dezember 2006 03:28 schrieb Sergio Monteiro Basto:
> On Wed, 2006-12-06 at 23:30 +0100, Ingo Molnar wrote:
> > Subject: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()
> > From: Ingo Molnar <mingo@elte.hu>
> Hi Ingo,
> Just curiosity ,have we this patch on 2.6.19-1.rt6 ?

No, its in rt7.

      Karsten

Re: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[Ingo Molnar]

* Len Brown <len.brown@intel.com> wrote:

> c. disable the NMI whenever the ACPI interpeter is running
>    (who knows, maybe this isn't limited to the _INI case, but
>     could cause a hang at some other time -- only the
>     BIOS AML writers knows....)

i have tested this by forcing the NMI frequency to 10,000 per second, 
and never saw any other problem. So at least this particular laptop 
should be OK.

So i /think/ this should be enough - the _INI case should be limited to 
bootup - or can it trigger during module load too? The IO-APIC based NMI 
watchdog should really only involve the southbridge (whose 
initialization package has this problem, in my guesstimation - do you 
agree?) and not random other devices - so once we have booted up we 
should be fine from this particular issue. acpi_nmi_disable()/enable() 
does a cross-IPI to all CPUs, so it can be quite heavy-handed - i'm not 
sure we want it for every interpreter invocation.

	Ingo

Re: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[Ingo Molnar]

* Karsten Wiese <fzu@wemgehoertderstaat.de> wrote:

> Am Donnerstag, 7. Dezember 2006 03:28 schrieb Sergio Monteiro Basto:
> > On Wed, 2006-12-06 at 23:30 +0100, Ingo Molnar wrote:
> > > Subject: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()
> > > From: Ingo Molnar <mingo@elte.hu>
> > Hi Ingo,
> > Just curiosity ,have we this patch on 2.6.19-1.rt6 ?
> 
> No, its in rt7.

yeah, it's in -rt7. [ -rt6 is more than 1 day old now, so it's truly 
ancient stuff ;-) ]

	Ingo

Re: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> * Karsten Wiese <fzu@wemgehoertderstaat.de> wrote:
> 
> > Am Donnerstag, 7. Dezember 2006 03:28 schrieb Sergio Monteiro Basto:
> > > On Wed, 2006-12-06 at 23:30 +0100, Ingo Molnar wrote:
> > > > Subject: [patch] ACPI, i686, x86_64: fix laptop bootup hang in init_acpi()
> > > > From: Ingo Molnar <mingo@elte.hu>
> > > Hi Ingo,
> > > Just curiosity ,have we this patch on 2.6.19-1.rt6 ?
> > 
> > No, its in rt7.
> 
> yeah, it's in -rt7. [ -rt6 is more than 1 day old now, so it's truly 
> ancient stuff ;-) ]

did i really say -rt7? The ancient -rt7 stuff is 12+ hours old already, 
and there's the 1 minute old -rt8 available (patch and yum rpms too). 
Sheesh ... ;-)

	Ingo

[patch] x86_64: do not enable the NMI watchdog by default

[Ingo Molnar]

* Len Brown <len.brown@intel.com> wrote:

> Personally I have never been a big fan of having the NMI watchdog 
> running by default on all systems -- but Andi insists that it helps 
> him debug failures, so tick it does...

enabling it by default was IMO a really bad decision and it needs to be 
undone via the patch attached further below.

If Andi wants to debug stuff via the NMI wachdog, he should use the 
nmi_watchdog=2 boot option: next to the tty=ttyS0 serial console 
options, initcall_debug, apic=debug, earlyprintk and myriads of other 
kernel-hackers-only boot options that we all use to 'help debug 
failures' ...

also, lock debugging facilities catch lockup possibilities (and actual 
lockups) alot more efficiently, i cannot remember the last time the NMI 
watchdog caught anything on my boxes without some other facility not 
triggering first. (and i have it enabled everywhere)

I have run a quick analysis over all locking related crashes i triggered 
on one particular testbox over the past 2 weeks. Out of 26 separate lock 
related incidents spread out equally over that timeframe (out of 497 
bootups on this box), this was the distribution of debugging facilities 
that caught the bugs:

      1  BUG: spinlock lockup on
      1  [ INFO: inconsistent lock state ]
      2  BUG: scheduling while atomic
      2  [ BUG: bad unlock balance detected! ]
      2  BUG: sleeping function called from invalid context
      6  BUG: scheduling with irqs disabled
      5  [ INFO: hard-safe -> hard-unsafe lock order detected ]
      7  BUG: using smp_processor_id() in preemptible [] code

8 were caught by lockdep, 8 by atomicity checks in the scheduler, 7 by 
DEBUG_PREEMPT and 1 by DEBUG_SPINLOCK.

Note: zero were caught by the NMI watchdog, and i run the NMI watchdog 
enabled by default on all architectures, and i have serial logging of 
everything.

but even for the typical distro kernel and for the typical user, the NMI 
watchdog is normally useless, because NMI lockups rarely make it into 
the syslog and X just locks up without showing anything on the screen.

	Ingo

----------------------->
Subject: [patch] x86_64: do not enable the NMI watchdog by default
From: Ingo Molnar <mingo@elte.hu>

do not enable the NMI watchdog by default. Now that we have lockdep i 
cannot remember the last time it caught a real bug, but the NMI watchdog 
can /cause/ problems. Furthermore, to the typical user, an NMI watchdog 
assert results in a total lockup anyway (if under X). In that sense, all 
that the NMI watchdog does is that it makes the system /less/ stable and 
/less/ debuggable.

people can still enable it either after bootup via:

   echo 1 > /proc/sys/kernel/nmi

or via the nmi_watchdog=1 or nmi_watchdog=2 boot options.

build and boot tested on an Athlon64 box.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86_64/kernel/apic.c    |    1 -
 arch/x86_64/kernel/io_apic.c |    5 +----
 arch/x86_64/kernel/nmi.c     |    2 +-
 arch/x86_64/kernel/smpboot.c |    1 -
 include/asm-x86_64/nmi.h     |    1 -
 5 files changed, 2 insertions(+), 8 deletions(-)

Index: linux/arch/x86_64/kernel/apic.c
===================================================================
--- linux.orig/arch/x86_64/kernel/apic.c
+++ linux/arch/x86_64/kernel/apic.c
@@ -443,7 +443,6 @@ void __cpuinit setup_local_APIC (void)
 			oldvalue, value);
 	}
 
-	nmi_watchdog_default();
 	setup_apic_nmi_watchdog(NULL);
 	apic_pm_activate();
 }
Index: linux/arch/x86_64/kernel/io_apic.c
===================================================================
--- linux.orig/arch/x86_64/kernel/io_apic.c
+++ linux/arch/x86_64/kernel/io_apic.c
@@ -1604,7 +1604,6 @@ static inline void check_timer(void)
 		 */
 		unmask_IO_APIC_irq(0);
 		if (!no_timer_check && timer_irq_works()) {
-			nmi_watchdog_default();
 			if (nmi_watchdog == NMI_IO_APIC) {
 				disable_8259A_irq(0);
 				setup_nmi();
@@ -1630,10 +1629,8 @@ static inline void check_timer(void)
 		setup_ExtINT_IRQ0_pin(apic2, pin2, vector);
 		if (timer_irq_works()) {
 			apic_printk(APIC_VERBOSE," works.\n");
-			nmi_watchdog_default();
-			if (nmi_watchdog == NMI_IO_APIC) {
+			if (nmi_watchdog == NMI_IO_APIC)
 				setup_nmi();
-			}
 			return;
 		}
 		/*
Index: linux/arch/x86_64/kernel/nmi.c
===================================================================
--- linux.orig/arch/x86_64/kernel/nmi.c
+++ linux/arch/x86_64/kernel/nmi.c
@@ -181,7 +181,7 @@ static __cpuinit inline int nmi_known_cp
 }
 
 /* Run after command line and cpu_init init, but before all other checks */
-void nmi_watchdog_default(void)
+static inline void nmi_watchdog_default(void)
 {
 	if (nmi_watchdog != NMI_DEFAULT)
 		return;
Index: linux/arch/x86_64/kernel/smpboot.c
===================================================================
--- linux.orig/arch/x86_64/kernel/smpboot.c
+++ linux/arch/x86_64/kernel/smpboot.c
@@ -866,7 +866,6 @@ static int __init smp_sanity_check(unsig
  */
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
-	nmi_watchdog_default();
 	current_cpu_data = boot_cpu_data;
 	current_thread_info()->cpu = 0;  /* needed? */
 	set_cpu_sibling_map(0);
Index: linux/include/asm-x86_64/nmi.h
===================================================================
--- linux.orig/include/asm-x86_64/nmi.h
+++ linux/include/asm-x86_64/nmi.h
@@ -59,7 +59,6 @@ extern void disable_timer_nmi_watchdog(v
 extern void enable_timer_nmi_watchdog(void);
 extern int nmi_watchdog_tick (struct pt_regs * regs, unsigned reason);
 
-extern void nmi_watchdog_default(void);
 extern int setup_nmi_watchdog(char *);
 
 extern atomic_t nmi_active;

Re: [patch] x86_64: do not enable the NMI watchdog by default

[Alan]

On Thu, 7 Dec 2006 13:11:35 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> or via the nmi_watchdog=1 or nmi_watchdog=2 boot options.
> 
> build and boot tested on an Athlon64 box.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

Acked-by: Alan Cox <alan@redhat.com>

Re: [patch] x86_64: do not enable the NMI watchdog by default

[Andi Kleen]

> and it needs to be  
> undone via the patch attached further below.

I disagree. And it has often saved my ass on 64bit. I

On 32bit it might be reevaluated -- i didn't expect that amount
of laptop firmware bugs triggered by it, but I'm not quite
ready to give up on that yet.

> If Andi wants to debug stuff via the NMI wachdog, he should use the 
> nmi_watchdog=2 boot option:

This means for most lockups which are hard to reproduce we don't 
get any backtrace.

And nmi_watchdog=2 is bad because it runs at HZ frequency 
and has quite high overhead.

> also, lock debugging facilities catch lockup possibilities (and actual 
> lockups) alot more efficiently, 

Production kernels don't have lock debugging enabled because it 
has far too much overhead.

> 8 were caught by lockdep, 8 by atomicity checks in the scheduler, 7 by 
> DEBUG_PREEMPT and 1 by DEBUG_SPINLOCK.

None of which is enabled on non debug kernels.
 
> Note: zero were caught by the NMI watchdog, and i run the NMI watchdog 
> enabled by default on all architectures, and i have serial logging of 
> everything.

Sure lock debugging will probably catch most of this earlier,
but we don't have it usually.

-Andi

Re: [patch] x86_64: do not enable the NMI watchdog by default

[Andrew Morton]

On Thu, 7 Dec 2006 12:30:11 +0000
Alan <alan@lxorguk.ukuu.org.uk> wrote:

> On Thu, 7 Dec 2006 13:11:35 +0100
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > or via the nmi_watchdog=1 or nmi_watchdog=2 boot options.
> > 
> > build and boot tested on an Athlon64 box.
> > 
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> Acked-by: Alan Cox <alan@redhat.com>

metoo.  I'm really struggling to recall an occasion on which the NMI
watchdog helped diagnose or fix a bug.  The usual scenario nowadays is that
I ask a reporter to enable NMI watchdog and for various reasons (mostly
mysterious) no useful information comes of it.

If it's causing machines to go down then the current tradeoff doesn't seem
right.

But _is_ it causing machines to go down, after the ACPI fix?

(the patch doesn't vaguely apply btw).

Re: [patch] x86_64: do not enable the NMI watchdog by default

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> (the patch doesn't vaguely apply btw).

patch below should apply to tail of current-ish -mm. Build and boot 
tested on x86_64.

	Ingo

---------------------->
Subject: [patch] x86_64: do not enable the NMI watchdog by default
From: Ingo Molnar <mingo@elte.hu>

do not enable the NMI watchdog by default. Now that we have
lockdep i cannot remember the last time it caught a real bug,
but the NMI watchdog can /cause/ problems. Furthermore, to the
typical user, an NMI watchdog assert results in a total lockup
anyway (if under X). In that sense, all that the NMI watchdog
does is that it makes the system /less/ stable and /less/
debuggable.

people can still enable it either after bootup via:

   echo 1 > /proc/sys/kernel/nmi

or via the nmi_watchdog=1 or nmi_watchdog=2 boot options.

build and boot tested on an Athlon64 box.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86_64/kernel/apic.c    |    1 -
 arch/x86_64/kernel/io_apic.c |    1 -
 arch/x86_64/kernel/nmi.c     |    2 +-
 arch/x86_64/kernel/smpboot.c |    1 -
 include/asm-x86_64/nmi.h     |    1 -
 5 files changed, 1 insertion(+), 5 deletions(-)

Index: linux-mm-genapic.q/arch/x86_64/kernel/apic.c
===================================================================
--- linux-mm-genapic.q.orig/arch/x86_64/kernel/apic.c
+++ linux-mm-genapic.q/arch/x86_64/kernel/apic.c
@@ -427,7 +427,6 @@ void __cpuinit setup_local_APIC (void)
 			oldvalue, value);
 	}
 
-	nmi_watchdog_default();
 	setup_apic_nmi_watchdog(NULL);
 	apic_pm_activate();
 }
Index: linux-mm-genapic.q/arch/x86_64/kernel/io_apic.c
===================================================================
--- linux-mm-genapic.q.orig/arch/x86_64/kernel/io_apic.c
+++ linux-mm-genapic.q/arch/x86_64/kernel/io_apic.c
@@ -1580,7 +1580,6 @@ static int try_apic_pin(int apic, int pi
 	 * Ok, does IRQ0 through the IOAPIC work?
 	 */
 	if (!no_timer_check && timer_irq_works()) {
-		nmi_watchdog_default();
 		if (nmi_watchdog == NMI_IO_APIC) {
 			disable_8259A_irq(0);
 			setup_nmi();
Index: linux-mm-genapic.q/arch/x86_64/kernel/nmi.c
===================================================================
--- linux-mm-genapic.q.orig/arch/x86_64/kernel/nmi.c
+++ linux-mm-genapic.q/arch/x86_64/kernel/nmi.c
@@ -183,7 +183,7 @@ static __cpuinit inline int nmi_known_cp
 }
 
 /* Run after command line and cpu_init init, but before all other checks */
-void nmi_watchdog_default(void)
+static inline void nmi_watchdog_default(void)
 {
 	if (nmi_watchdog != NMI_DEFAULT)
 		return;
Index: linux-mm-genapic.q/arch/x86_64/kernel/smpboot.c
===================================================================
--- linux-mm-genapic.q.orig/arch/x86_64/kernel/smpboot.c
+++ linux-mm-genapic.q/arch/x86_64/kernel/smpboot.c
@@ -1080,7 +1080,6 @@ static int __init smp_sanity_check(unsig
  */
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
-	nmi_watchdog_default();
 	current_cpu_data = boot_cpu_data;
 	current_thread_info()->cpu = 0;  /* needed? */
 	set_cpu_sibling_map(0);
Index: linux-mm-genapic.q/include/asm-x86_64/nmi.h
===================================================================
--- linux-mm-genapic.q.orig/include/asm-x86_64/nmi.h
+++ linux-mm-genapic.q/include/asm-x86_64/nmi.h
@@ -59,7 +59,6 @@ extern void disable_timer_nmi_watchdog(v
 extern void enable_timer_nmi_watchdog(void);
 extern int nmi_watchdog_tick (struct pt_regs * regs, unsigned reason);
 
-extern void nmi_watchdog_default(void);
 extern int setup_nmi_watchdog(char *);
 
 extern atomic_t nmi_active;

Re: [patch] x86_64: do not enable the NMI watchdog by default

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > (the patch doesn't vaguely apply btw).
> 
> patch below should apply to tail of current-ish -mm. Build and boot 
> tested on x86_64.

btw., lockdep noticed a locking breakage in netconsole, see the log 
below. My guess: dev_watchdog shouldnt be taking the lock without _bh.

	Ingo

--------------->
Calling initcall 0xffffffff8098f2e0: rtl8139_init_module+0x0/0x14()
8139too Fast Ethernet driver 0.9.28
ACPI: PCI Interrupt Link [APC2] enabled at IRQ 17
IOAPIC[0]: Set PCI routing entry (2-17 -> 0x59 -> IRQ 17 Mode:1 Active:1)
ACPI: PCI Interrupt 0000:05:07.0[A] -> Link [APC2] -> GSI 17 (level, low) -> IRQ 17
eth0: RealTek RTL8139 at 0xa000, 00:c0:df:03:68:5d, IRQ 17
eth0:  Identified 8139 chip type 'RTL-8139B'
Calling initcall 0xffffffff803ce560: init_netconsole+0x0/0x68()
netconsole: device eth0 not up yet, forcing it
eth0: link up, 100Mbps, full-duplex, lpa 0xC5E1
netconsole: carrier detect appears untrustworthy, waiting 4 seconds

=================================
[ INFO: inconsistent lock state ]
2.6.19-mm1 #4
---------------------------------
inconsistent {softirq-on-W} -> {in-softirq-W} usage.
swapper/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
 (&dev->_xmit_lock){-+..}, at: [<ffffffff80453151>] dev_watchdog+0x15/0xe0
{softirq-on-W} state was registered at:
  [<ffffffff80251078>] mark_lock+0x78/0x3cf
  [<ffffffff80251422>] mark_held_locks+0x53/0x71
  [<ffffffff802515ed>] trace_hardirqs_on+0x113/0x137
  [<ffffffff803cda5f>] rtl8139_poll+0x3c9/0x3ee
  [<ffffffff8044f03d>] netpoll_poll+0xa1/0x32f
  [<ffffffff8044ef44>] netpoll_send_skb+0xdf/0x137
  [<ffffffff8044f5b4>] netpoll_send_udp+0x263/0x270
  [<ffffffff803ce632>] write_msg+0x4c/0x7e
  [<ffffffff8023671b>] __call_console_drivers+0x5f/0x70
  [<ffffffff80236790>] _call_console_drivers+0x64/0x68
  [<ffffffff80236e6c>] release_console_sem+0x148/0x207
  [<ffffffff80237165>] register_console+0x1b1/0x1ba
  [<ffffffff803ce5b4>] init_netconsole+0x54/0x68
  [<ffffffff802071d9>] init+0x178/0x347
  [<ffffffff8020ab98>] child_rip+0xa/0x12
  [<ffffffffffffffff>] 0xffffffffffffffff
irq event stamp: 23912
hardirqs last  enabled at (23912): [<ffffffff804aedc5>] _spin_unlock_irq+0x28/0x52
hardirqs last disabled at (23911): [<ffffffff804aecec>] _spin_lock_irq+0xf/0x3e
softirqs last  enabled at (23896): [<ffffffff8023befd>] __do_softirq+0xdb/0xe4
softirqs last disabled at (23909): [<ffffffff8020af0c>] call_softirq+0x1c/0x30

other info that might help us debug this:
no locks held by swapper/0.

stack backtrace:

Call Trace:
 [<ffffffff8020b304>] dump_trace+0xc1/0x3eb
 [<ffffffff8020b667>] show_trace+0x39/0x57
 [<ffffffff8020b89c>] dump_stack+0x13/0x15
 [<ffffffff80250cff>] print_usage_bug+0x26b/0x27a
 [<ffffffff8025112b>] mark_lock+0x12b/0x3cf
 [<ffffffff80251b0b>] __lock_acquire+0x3c0/0xa0f
 [<ffffffff80252426>] lock_acquire+0x4d/0x67
 [<ffffffff804ae747>] _spin_lock+0x2c/0x38
 [<ffffffff80453151>] dev_watchdog+0x15/0xe0
 [<ffffffff802401d9>] run_timer_softirq+0x167/0x1db
 [<ffffffff8023be84>] __do_softirq+0x62/0xe4
 [<ffffffff8020af0c>] call_softirq+0x1c/0x30
 [<ffffffff8020c6a2>] do_softirq+0x36/0x9c
 [<ffffffff8023bb47>] irq_exit+0x45/0x51
 [<ffffffff80219d79>] smp_apic_timer_interrupt+0x49/0x5c
 [<ffffffff8020a9bb>] apic_timer_interrupt+0x6b/0x70
 [<ffffffff80208823>] default_idle+0x36/0x50
 [<ffffffff802088d8>] cpu_idle+0x9b/0xd4
 [<ffffffff802193f9>] start_secondary+0x498/0x4a7

netconsole: network logging started
Calling initcall 0xffffffff803ce8f4: aec62xx_ide_init+0x0/0x14()
Calling initcall 0xffffffff803cf093: ali15x3_ide_init+0x0/0x14()
Calling initcall 0xffffffff803d086b: amd74xx_ide_init+0x0/0x14()

[patch] net: dev_watchdog() locking fix

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > patch below should apply to tail of current-ish -mm. Build and boot 
> > tested on x86_64.
> 
> btw., lockdep noticed a locking breakage in netconsole, see the log 
> below. My guess: dev_watchdog shouldnt be taking the lock without _bh.

fallout of the recent big networking merge i guess. Tested fix below. 
David, Herbert, do you agree with it, or is it a false positive?

	Ingo

------------->
Subject: [patch] net: dev_watchdog() locking fix
From: Ingo Molnar <mingo@elte.hu>

lockdep noticed the following bug:

=================================
[ INFO: inconsistent lock state ]
2.6.19-mm1 #4
---------------------------------
inconsistent {softirq-on-W} -> {in-softirq-W} usage.
swapper/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
 (&dev->_xmit_lock){-+..}, at: [<ffffffff80453151>] dev_watchdog+0x15/0xe0
{softirq-on-W} state was registered at:
  [<ffffffff80251078>] mark_lock+0x78/0x3cf
  [<ffffffff80251422>] mark_held_locks+0x53/0x71
  [<ffffffff802515ed>] trace_hardirqs_on+0x113/0x137
  [<ffffffff803cda5f>] rtl8139_poll+0x3c9/0x3ee
  [<ffffffff8044f03d>] netpoll_poll+0xa1/0x32f
  [<ffffffff8044ef44>] netpoll_send_skb+0xdf/0x137
  [<ffffffff8044f5b4>] netpoll_send_udp+0x263/0x270
  [<ffffffff803ce632>] write_msg+0x4c/0x7e
  [<ffffffff8023671b>] __call_console_drivers+0x5f/0x70
  [<ffffffff80236790>] _call_console_drivers+0x64/0x68
  [<ffffffff80236e6c>] release_console_sem+0x148/0x207
  [<ffffffff80237165>] register_console+0x1b1/0x1ba
  [<ffffffff803ce5b4>] init_netconsole+0x54/0x68
  [<ffffffff802071d9>] init+0x178/0x347
  [<ffffffff8020ab98>] child_rip+0xa/0x12
  [<ffffffffffffffff>] 0xffffffffffffffff
irq event stamp: 23912
hardirqs last  enabled at (23912): [<ffffffff804aedc5>] _spin_unlock_irq+0x28/0x52
hardirqs last disabled at (23911): [<ffffffff804aecec>] _spin_lock_irq+0xf/0x3e
softirqs last  enabled at (23896): [<ffffffff8023befd>] __do_softirq+0xdb/0xe4
softirqs last disabled at (23909): [<ffffffff8020af0c>] call_softirq+0x1c/0x30

other info that might help us debug this:
no locks held by swapper/0.

stack backtrace:

Call Trace:
 [<ffffffff8020b304>] dump_trace+0xc1/0x3eb
 [<ffffffff8020b667>] show_trace+0x39/0x57
 [<ffffffff8020b89c>] dump_stack+0x13/0x15
 [<ffffffff80250cff>] print_usage_bug+0x26b/0x27a
 [<ffffffff8025112b>] mark_lock+0x12b/0x3cf
 [<ffffffff80251b0b>] __lock_acquire+0x3c0/0xa0f
 [<ffffffff80252426>] lock_acquire+0x4d/0x67
 [<ffffffff804ae747>] _spin_lock+0x2c/0x38
 [<ffffffff80453151>] dev_watchdog+0x15/0xe0
 [<ffffffff802401d9>] run_timer_softirq+0x167/0x1db
 [<ffffffff8023be84>] __do_softirq+0x62/0xe4
 [<ffffffff8020af0c>] call_softirq+0x1c/0x30
 [<ffffffff8020c6a2>] do_softirq+0x36/0x9c
 [<ffffffff8023bb47>] irq_exit+0x45/0x51
 [<ffffffff80219d79>] smp_apic_timer_interrupt+0x49/0x5c
 [<ffffffff8020a9bb>] apic_timer_interrupt+0x6b/0x70
 [<ffffffff80208823>] default_idle+0x36/0x50
 [<ffffffff802088d8>] cpu_idle+0x9b/0xd4
 [<ffffffff802193f9>] start_secondary+0x498/0x4a7

taking the lock _bh safe fixes it for me.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 net/sched/sch_generic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-mm-genapic.q/net/sched/sch_generic.c
===================================================================
--- linux-mm-genapic.q.orig/net/sched/sch_generic.c
+++ linux-mm-genapic.q/net/sched/sch_generic.c
@@ -197,7 +197,7 @@ static void dev_watchdog(unsigned long a
 {
 	struct net_device *dev = (struct net_device *)arg;
 
-	netif_tx_lock(dev);
+	netif_tx_lock_bh(dev);
 	if (dev->qdisc != &noop_qdisc) {
 		if (netif_device_present(dev) &&
 		    netif_running(dev) &&
@@ -213,7 +213,7 @@ static void dev_watchdog(unsigned long a
 				dev_hold(dev);
 		}
 	}
-	netif_tx_unlock(dev);
+	netif_tx_unlock_bh(dev);
 
 	dev_put(dev);
 }

Re: [patch] net: dev_watchdog() locking fix

[Herbert Xu]

On Thu, Dec 07, 2006 at 09:55:22PM +0100, Ingo Molnar wrote:
> 
> fallout of the recent big networking merge i guess. Tested fix below. 
> David, Herbert, do you agree with it, or is it a false positive?

I agree that this is a bug, but the fix is in the wrong spot.  The
dev_watchdog function already runs in softirq context so it doesn't
need to disable BH.

You can almost be guaranteed that if netpoll is involved in a bug
then it should be fixed :)

In this case, it's taking the tx lock in process context which is
not allowed.  So it should disable BH before taking the tx lock.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch] net: dev_watchdog() locking fix

[Andrew Morton]

On Fri, 8 Dec 2006 08:06:57 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Thu, Dec 07, 2006 at 09:55:22PM +0100, Ingo Molnar wrote:
> > 
> > fallout of the recent big networking merge i guess. Tested fix below. 
> > David, Herbert, do you agree with it, or is it a false positive?
> 
> I agree that this is a bug, but the fix is in the wrong spot.  The
> dev_watchdog function already runs in softirq context so it doesn't
> need to disable BH.
> 
> You can almost be guaranteed that if netpoll is involved in a bug
> then it should be fixed :)
> 
> In this case, it's taking the tx lock in process context which is
> not allowed.  So it should disable BH before taking the tx lock.
> 

Like this?

	/* don't get messages out of order, and no recursion */
	if (skb_queue_len(&npinfo->txq) == 0 &&
		    npinfo->poll_owner != smp_processor_id()) {
		local_bh_disable();	/* Where's netif_tx_trylock_bh()? */
		if (netif_tx_trylock(dev)) {
			/* try until next clock tick */
			for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
					tries > 0; --tries) {
				if (!netif_queue_stopped(dev))
					status = dev->hard_start_xmit(skb, dev);

				if (status == NETDEV_TX_OK)
					break;

				/* tickle device maybe there is some cleanup */
				netpoll_poll(np);

				udelay(USEC_PER_POLL);
			}
			netif_tx_unlock(dev);
		}
		local_bh_enable();
	}


--- a/net/core/netpoll.c~netpoll-locking-fix
+++ a/net/core/netpoll.c
@@ -242,22 +242,26 @@ static void netpoll_send_skb(struct netp
 
 	/* don't get messages out of order, and no recursion */
 	if (skb_queue_len(&npinfo->txq) == 0 &&
-	    npinfo->poll_owner != smp_processor_id() &&
-	    netif_tx_trylock(dev)) {
-		/* try until next clock tick */
-		for (tries = jiffies_to_usecs(1)/USEC_PER_POLL; tries > 0; --tries) {
-			if (!netif_queue_stopped(dev))
-				status = dev->hard_start_xmit(skb, dev);
+		    npinfo->poll_owner != smp_processor_id()) {
+		local_bh_disable();	/* Where's netif_tx_trylock_bh()? */
+		if (netif_tx_trylock(dev)) {
+			/* try until next clock tick */
+			for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
+					tries > 0; --tries) {
+				if (!netif_queue_stopped(dev))
+					status = dev->hard_start_xmit(skb, dev);
 
-			if (status == NETDEV_TX_OK)
-				break;
+				if (status == NETDEV_TX_OK)
+					break;
 
-			/* tickle device maybe there is some cleanup */
-			netpoll_poll(np);
+				/* tickle device maybe there is some cleanup */
+				netpoll_poll(np);
 
-			udelay(USEC_PER_POLL);
+				udelay(USEC_PER_POLL);
+			}
+			netif_tx_unlock(dev);
 		}
-		netif_tx_unlock(dev);
+		local_bh_enable();
 	}
 
 	if (status != NETDEV_TX_OK) {
_

Re: [patch] net: dev_watchdog() locking fix

[Herbert Xu]

On Fri, Dec 08, 2006 at 03:19:02PM -0800, Andrew Morton wrote:
> 
> Like this?
> 
> 	/* don't get messages out of order, and no recursion */
> 	if (skb_queue_len(&npinfo->txq) == 0 &&
> 		    npinfo->poll_owner != smp_processor_id()) {
> 		local_bh_disable();	/* Where's netif_tx_trylock_bh()? */
> 		if (netif_tx_trylock(dev)) {
> 			/* try until next clock tick */
> 			for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
> 					tries > 0; --tries) {
> 				if (!netif_queue_stopped(dev))
> 					status = dev->hard_start_xmit(skb, dev);
> 
> 				if (status == NETDEV_TX_OK)
> 					break;
> 
> 				/* tickle device maybe there is some cleanup */
> 				netpoll_poll(np);
> 
> 				udelay(USEC_PER_POLL);
> 			}
> 			netif_tx_unlock(dev);
> 		}
> 		local_bh_enable();
> 	}

Looks good to me.  Thanks Andrew!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch] net: dev_watchdog() locking fix

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 9 Dec 2006 10:59:52 +1100

> On Fri, Dec 08, 2006 at 03:19:02PM -0800, Andrew Morton wrote:
> > 
> > Like this?
> > 
> > 	/* don't get messages out of order, and no recursion */
> > 	if (skb_queue_len(&npinfo->txq) == 0 &&
> > 		    npinfo->poll_owner != smp_processor_id()) {
> > 		local_bh_disable();	/* Where's netif_tx_trylock_bh()? */
> > 		if (netif_tx_trylock(dev)) {
> > 			/* try until next clock tick */
> > 			for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
> > 					tries > 0; --tries) {
> > 				if (!netif_queue_stopped(dev))
> > 					status = dev->hard_start_xmit(skb, dev);
> > 
> > 				if (status == NETDEV_TX_OK)
> > 					break;
> > 
> > 				/* tickle device maybe there is some cleanup */
> > 				netpoll_poll(np);
> > 
> > 				udelay(USEC_PER_POLL);
> > 			}
> > 			netif_tx_unlock(dev);
> > 		}
> > 		local_bh_enable();
> > 	}
> 
> Looks good to me.  Thanks Andrew!

I've applied this patch, thanks a lot.

Re: [patch] net: dev_watchdog() locking fix

[Andrew Morton]

On Sat, 09 Dec 2006 14:02:05 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Sat, 9 Dec 2006 10:59:52 +1100
> 
> > On Fri, Dec 08, 2006 at 03:19:02PM -0800, Andrew Morton wrote:
> > > 
> > > Like this?
> > > 
> > > 	/* don't get messages out of order, and no recursion */
> > > 	if (skb_queue_len(&npinfo->txq) == 0 &&
> > > 		    npinfo->poll_owner != smp_processor_id()) {
> > > 		local_bh_disable();	/* Where's netif_tx_trylock_bh()? */
> > > 		if (netif_tx_trylock(dev)) {
> > > 			/* try until next clock tick */
> > > 			for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
> > > 					tries > 0; --tries) {
> > > 				if (!netif_queue_stopped(dev))
> > > 					status = dev->hard_start_xmit(skb, dev);
> > > 
> > > 				if (status == NETDEV_TX_OK)
> > > 					break;
> > > 
> > > 				/* tickle device maybe there is some cleanup */
> > > 				netpoll_poll(np);
> > > 
> > > 				udelay(USEC_PER_POLL);
> > > 			}
> > > 			netif_tx_unlock(dev);
> > > 		}
> > > 		local_bh_enable();
> > > 	}
> > 
> > Looks good to me.  Thanks Andrew!
> 
> I've applied this patch, thanks a lot.

It spits a nasty during bringup

e1000: eth0: e1000_probe: Intel(R) PRO/1000 Network Connection
forcedeth.c: Reverse Engineered nForce ethernet driver. Version 0.59.
netconsole: device eth0 not up yet, forcing it
e1000: eth0: e1000_watchdog: NIC Link is Up 100 Mbps Full Duplex
WARNING (!__warned) at kernel/softirq.c:137 local_bh_enable()

Call Trace:
 [<ffffffff80235baf>] local_bh_enable+0x41/0xa3
 [<ffffffff8045ab8e>] netpoll_send_skb+0x116/0x144
 [<ffffffff8045b1ee>] netpoll_send_udp+0x263/0x271
 [<ffffffff803d41ec>] write_msg+0x42/0x5e
 [<ffffffff80230c9b>] __call_console_drivers+0x5f/0x70
 [<ffffffff80230d19>] _call_console_drivers+0x6d/0x71
 [<ffffffff802313f0>] release_console_sem+0x148/0x1ec
 [<ffffffff802316ce>] register_console+0x1b1/0x1ba
 [<ffffffff803d4178>] init_netconsole+0x54/0x68
 [<ffffffff802071ae>] init+0x152/0x308
 [<ffffffff804dac8b>] _spin_unlock_irq+0x14/0x30
 [<ffffffff8022c15e>] schedule_tail+0x43/0x9f
 [<ffffffff8020a758>] child_rip+0xa/0x12
 [<ffffffff8020705c>] init+0x0/0x308
 [<ffffffff8020a74e>] child_rip+0x0/0x12

because local irqs are disabled.  

Re: [patch] net: dev_watchdog() locking fix

[Herbert Xu]

On Sun, Dec 10, 2006 at 11:45:08PM -0800, Andrew Morton wrote:
> 
> It spits a nasty during bringup
> 
> e1000: eth0: e1000_probe: Intel(R) PRO/1000 Network Connection
> forcedeth.c: Reverse Engineered nForce ethernet driver. Version 0.59.
> netconsole: device eth0 not up yet, forcing it
> e1000: eth0: e1000_watchdog: NIC Link is Up 100 Mbps Full Duplex
> WARNING (!__warned) at kernel/softirq.c:137 local_bh_enable()

Normally networking isn't invoked with interrupts turned off, but
I suppose we don't have a choice here.  This is unique being a
place where you can get called with BH on, off, or IRQs off.

Given that this is only used for printk, the easiest solution is
probably just to disable local IRQs instead of BH.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch] net: dev_watchdog() locking fix

[Ingo Molnar]

* Herbert Xu <herbert@gondor.apana.org.au> wrote:

> > It spits a nasty during bringup
> > 
> > e1000: eth0: e1000_probe: Intel(R) PRO/1000 Network Connection
> > forcedeth.c: Reverse Engineered nForce ethernet driver. Version 0.59.
> > netconsole: device eth0 not up yet, forcing it
> > e1000: eth0: e1000_watchdog: NIC Link is Up 100 Mbps Full Duplex
> > WARNING (!__warned) at kernel/softirq.c:137 local_bh_enable()
> 
> Normally networking isn't invoked with interrupts turned off, but I 
> suppose we don't have a choice here.  This is unique being a place 
> where you can get called with BH on, off, or IRQs off.
> 
> Given that this is only used for printk, the easiest solution is 
> probably just to disable local IRQs instead of BH.

yeah. local_bh_enable() can execute pending softirqs and that warning 
protects us against doing that from within irqs-off sections.

	Ingo

Re: [patch] net: dev_watchdog() locking fix

[Andrew Morton]

On Mon, 11 Dec 2006 18:51:11 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Sun, Dec 10, 2006 at 11:45:08PM -0800, Andrew Morton wrote:
> > 
> > It spits a nasty during bringup
> > 
> > e1000: eth0: e1000_probe: Intel(R) PRO/1000 Network Connection
> > forcedeth.c: Reverse Engineered nForce ethernet driver. Version 0.59.
> > netconsole: device eth0 not up yet, forcing it
> > e1000: eth0: e1000_watchdog: NIC Link is Up 100 Mbps Full Duplex
> > WARNING (!__warned) at kernel/softirq.c:137 local_bh_enable()
> 
> Normally networking isn't invoked with interrupts turned off, but
> I suppose we don't have a choice here.  This is unique being a
> place where you can get called with BH on, off, or IRQs off.
> 
> Given that this is only used for printk, the easiest solution is
> probably just to disable local IRQs instead of BH.
> 

I'll try that.  I wonder what will explode now..