[PATCH] Export touch_nmi_watchdog

[PATCH] Export touch_nmi_watchdog

[Andi Kleen]

Sometimes drivers do long things with interrupt off and the NMI watchdog
triggers quickly. This mostly happens in error handling. It would be 
better to fix the drivers to sleep in this case, but it's not always
possible or too much work.

This patch exports touch_nmi_watchdog so that they can at least be 
fixed to not trigger the watchdog.

Matches a similar patch for x86-64.

-Andi

diff -burpN -X ../KDIFX linux/arch/i386/kernel/nmi.c linux-2.6.0test2-amd64/arch/i386/kernel/nmi.c
--- linux/arch/i386/kernel/nmi.c	2003-07-18 02:40:03.000000000 +0200
+++ linux-2.6.0test2-work/arch/i386/kernel/nmi.c	2003-08-03 12:13:48.000000000 +0200
@@ -455,3 +455,4 @@ EXPORT_SYMBOL(disable_lapic_nmi_watchdog
 EXPORT_SYMBOL(enable_lapic_nmi_watchdog);
 EXPORT_SYMBOL(disable_timer_nmi_watchdog);
 EXPORT_SYMBOL(enable_timer_nmi_watchdog);
+EXPORT_SYMBOL(touch_nmi_watchdog);

Re: [PATCH] Export touch_nmi_watchdog

[Andrew Morton]

Andi Kleen <ak@muc.de> wrote:
>
> Sometimes drivers do long things with interrupt off and the NMI watchdog
>  triggers quickly. This mostly happens in error handling. It would be 
>  better to fix the drivers to sleep in this case, but it's not always
>  possible or too much work.

yup.

Do we need an mdelay_while_touching_nmi_watchdog() variant?

Re: [PATCH] Export touch_nmi_watchdog

[Andi Kleen]

On Tue, Aug 05, 2003 at 12:38:11PM -0700, Andrew Morton wrote:
> Andi Kleen <ak@muc.de> wrote:
> >
> > Sometimes drivers do long things with interrupt off and the NMI watchdog
> >  triggers quickly. This mostly happens in error handling. It would be 
> >  better to fix the drivers to sleep in this case, but it's not always
> >  possible or too much work.
> 
> yup.
> 
> Do we need an mdelay_while_touching_nmi_watchdog() variant?

Maybe that would be too encoraging for broken code. 

Admittedly I did that by hand for the MPT fusion driver (which currently 
triggers the watchdog when it gets into any error handling situation) 
This especially hurts on x86-64 which runs the watchdog by default. 
But it's strictly a bad hack, not a good interface.

-Andi

Re: [PATCH] Export touch_nmi_watchdog

[Arjan van de Ven]

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

On Tue, 2003-08-05 at 22:08, Andi Kleen wrote:
> On Tue, Aug 05, 2003 at 12:38:11PM -0700, Andrew Morton wrote:
> > Andi Kleen <ak@muc.de> wrote:
> > >
> > > Sometimes drivers do long things with interrupt off and the NMI watchdog
> > >  triggers quickly. This mostly happens in error handling. It would be 
> > >  better to fix the drivers to sleep in this case, but it's not always
> > >  possible or too much work.
> > 
> > yup.
> > 
> > Do we need an mdelay_while_touching_nmi_watchdog() variant?
> 
> Maybe that would be too encoraging for broken code. 
> 
> Admittedly I did that by hand for the MPT fusion driver (which currently 
> triggers the watchdog when it gets into any error handling situation) 
> This especially hurts on x86-64 which runs the watchdog by default. 
> But it's strictly a bad hack, not a good interface.

having a more generic/portable "trigger_watchdog" function would be
better then, such that ALL watchdogs, and not just the NMI one can hook
into this

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Export touch_nmi_watchdog

[Linus Torvalds]

On 5 Aug 2003, Arjan van de Ven wrote:
> 
> having a more generic/portable "trigger_watchdog" function would be
> better then, such that ALL watchdogs, and not just the NMI one can hook
> into this

Why are we working around broken drivers?

I say:
 - either fix the driver
or
 - disable the watchdog entirely.

I don't see any point at all to touch_xxx_watchdog() from a driver.

		Linus

Re: [PATCH] Export touch_nmi_watchdog

[Arjan van de Ven]

On Tue, Aug 05, 2003 at 01:25:09PM -0700, Linus Torvalds wrote:
> 
> On 5 Aug 2003, Arjan van de Ven wrote:
> > 
> > having a more generic/portable "trigger_watchdog" function would be
> > better then, such that ALL watchdogs, and not just the NMI one can hook
> > into this
> 
> Why are we working around broken drivers?
> 
> I say:
>  - either fix the driver
> or
>  - disable the watchdog entirely.
> 
> I don't see any point at all to touch_xxx_watchdog() from a driver.

In principle you are soooo right. Just that it sometimes is HARD to fix
such long delays... I remember working on the qla2x00 driver to fix that ;(
Ok "it's hard" is no excuse. 

Re: [PATCH] Export touch_nmi_watchdog

[Andi Kleen]

On Tue, Aug 05, 2003 at 01:25:09PM -0700, Linus Torvalds wrote:
> 
> On 5 Aug 2003, Arjan van de Ven wrote:
> > 
> > having a more generic/portable "trigger_watchdog" function would be
> > better then, such that ALL watchdogs, and not just the NMI one can hook
> > into this
> 
> Why are we working around broken drivers?

Because they're a hard to ignore reality? 

> 
> I say:
>  - either fix the driver

Would be quite a big project for the fusion driver. It's very big and 
complicated.

Of course I would like to fix it, but I see no chance at all to ever
find enough time to do that properly.

> or
>  - disable the watchdog entirely.

That would be a pity because it is very useful to find other problems.

-Andi

Re: [PATCH] Export touch_nmi_watchdog

[Tim Hockin]

> > >  - either fix the driver
> > > or
> > >  - disable the watchdog entirely.
> > 
> > In principle you are soooo right. Just that it sometimes is HARD to fix
> > such long delays...
> 
> That's why I said "disable the watchdog".

> But since the whole _point_ of watchdogging is to find places like this, 
> when you paper over _these_ symptoms, you end up killing the whole idea.
> 
> Which is why I'd suggest making it a more conscious decision: just turn
> off watchdog support. And if somebody needs watchdog support with a broken 
> driver, maybe, just _maybe_, he'll find the energy to fix the frigging 
> thing.

We had the same situation a while back (2.2.x, early 2.4.x era) with systems
that had (have, they still exist) a 1 second WD tied to the hard RESET line.
1 second with interrupts off == reboot.  Let me tell you, we found a LOT of
places where there were problems.  Network drivers, routing stuff, SCSI,
etc.  We fixed some, and hacked around some in this same manner.

Now, if I have a chance to port Cobalt crap to 2.6, we'll see how many of
our changes were merged, in concept or in code.

Tim

Re: [PATCH] Export touch_nmi_watchdog

[Linus Torvalds]

On Tue, 5 Aug 2003, Arjan van de Ven wrote:
> On Tue, Aug 05, 2003 at 01:25:09PM -0700, Linus Torvalds wrote:
> > 
> >  - either fix the driver
> > or
> >  - disable the watchdog entirely.
> 
> In principle you are soooo right. Just that it sometimes is HARD to fix
> such long delays...

That's why I said "disable the watchdog".

The thing is, if you start doing things like this in drivers, then you 
just hide the problem to the point where users end up not even being 
_aware_ of the problem. 

Do you _really_ think most users will think of "Oh, let's grep for
'touch_nmi_watchdog' in that driver" when they have latency issues?

No. Obviously they won't. Instead, they'll scratch their head about
occasional bad packet routing latency, report it as a networking bug, and
just generally look in the wrong place. And enabling lockup debugging will
do zero for them.

So this is a case of trying to paper over the symptoms. Which is perfectly 
ok in the sense that "we don't have the resources to fix it right now, so 
let's just give it two aspirins and ask it to call in the morning". That's 
fine.

But since the whole _point_ of watchdogging is to find places like this, 
when you paper over _these_ symptoms, you end up killing the whole idea.

Which is why I'd suggest making it a more conscious decision: just turn
off watchdog support. And if somebody needs watchdog support with a broken 
driver, maybe, just _maybe_, he'll find the energy to fix the frigging 
thing.

Otherwise this will just keep on expanding. 

		Linus

Re: [PATCH] Export touch_nmi_watchdog

[Andi Kleen]

> having a more generic/portable "trigger_watchdog" function would be
> better then, such that ALL watchdogs, and not just the NMI one can hook
> into this

Well the function is already used and it's just a name. You could always
hook other watchdogs into it too.

-Andi

Re: [PATCH] Export touch_nmi_watchdog

[Andi Kleen]

> Otherwise this will just keep on expanding. 

It does expand on i386 exactly because the watchdog is disabled by default.

Looks like a mistake to me. It should be on because having usable backtraces
on a deadlock/hang is useful enough that it outweights any other possible
disadvantages. That's especially true for kernels out there at user's boxes,
not just special debugging kernels run by developers.

[if there should be any hardware where it doesn't work it should be blacklisted
there]

-Andi

Re: [PATCH] Export touch_nmi_watchdog

[Arjan van de Ven]

On Tue, Aug 05, 2003 at 11:14:16PM +0200, Andi Kleen wrote:
> > Otherwise this will just keep on expanding. 
> 
> It does expand on i386 exactly because the watchdog is disabled by default.
> 
> Looks like a mistake to me. It should be on because having usable backtraces
> on a deadlock/hang is useful enough that it outweights any other possible
> disadvantages. That's especially true for kernels out there at user's boxes,
> not just special debugging kernels run by developers.
> 
> [if there should be any hardware where it doesn't work it should be blacklisted
> there]

the reason it's off is that certain IBM bioses corrupt the eax register on
NMI's when they collide with smm stuff... You'd be surprised how
tolerant x86 is against such corruptions... but not 100%  :)

Re: [PATCH] Export touch_nmi_watchdog

[Andi Kleen]

On Tue, Aug 05, 2003 at 09:15:54PM +0000, Arjan van de Ven wrote:
> On Tue, Aug 05, 2003 at 11:14:16PM +0200, Andi Kleen wrote:
> > > Otherwise this will just keep on expanding. 
> > 
> > It does expand on i386 exactly because the watchdog is disabled by default.
> > 
> > Looks like a mistake to me. It should be on because having usable backtraces
> > on a deadlock/hang is useful enough that it outweights any other possible
> > disadvantages. That's especially true for kernels out there at user's boxes,
> > not just special debugging kernels run by developers.
> > 
> > [if there should be any hardware where it doesn't work it should be blacklisted
> > there]
> 
> the reason it's off is that certain IBM bioses corrupt the eax register on
> NMI's when they collide with smm stuff... You'd be surprised how
> tolerant x86 is against such corruptions... but not 100%  :)

That could be catched by a dmi_scan.c entry ?

-Andi

Re: [PATCH] Export touch_nmi_watchdog

[Linus Torvalds]

On 5 Aug 2003, Andi Kleen wrote:
>
> > Otherwise this will just keep on expanding. 
> 
> It does expand on i386 exactly because the watchdog is disabled by default.

It's flaky enough that it _shouldn't_ be enabled by default. 

Oh, it's easy for other architectures that don't support the same wide
range of hardware to look down on it, but the thing is, you don't have the
same variability in firmware, interrupt controllers, system buses and
CPU's.

And like it or not, but that "richness" is what makes the x86 so 
successful. 

But my point is that once you find a bug you should not just paper it over 
and make sure that nobody finds it ever again. That's counter-productive. 
It's especially counter-productive if you do it in a way where other 
driver writers may well end up _copying_ the code that hides the bug. Just 
because they don't know any better.

So my argument is that we'd actually be a whole lot better off just adding
something like

	#ifdef CONFIG_WATCHDOG
	#warning This driver does bad things and will not work
	#endif

around the section that you found using the watchdog. Or something like 
this in the init routine for the driver:

	disable_watchdog();

which will disable the watchdog at run-time AND put a huge big bright 
printk() on the screen saying "watchdog is not usable with this driver". 
Again, to make people _aware_ of the problem when they hit it.

That way, next time somebody comes around and decides to nose around in
the driver, maybe they will fix it. Or when somebody else uses the driver
as a basis for their "new and improved" version, they'll take one look at
that, and say "oh, I won't make that mistake this time through".

In other words, we should make it clear that it is a CRIME to need to ping 
the watchdog. Because if you disable enough interrupts to trigger the 
watchdog, you _are_ doing bad things that are potentially visible to the 
user.

So let the user know. Don't just silently say "let's kick the watchdog".

		Linus

Re: [PATCH] Export touch_nmi_watchdog

[Andi Kleen]

On Tue, Aug 05, 2003 at 03:14:00PM -0700, Linus Torvalds wrote:
> 	#ifdef CONFIG_WATCHDOG
> 	#warning This driver does bad things and will not work
> 	#endif

Well the problem is that many of my multiple CPU testboxes have a fusion
controller (it's the standard on board chip on the AMD Quartet and Newisys
systems). 

At least for my testing it would be quite inconvenient to not
have the watchdog. I also don't see anybody comming around and fixing
the SCSI error handler of that driver (scsi error handlers seem
to be always very bad code, undoubtedly because it's a ugly problem)

(fortunately errors happen only infrequently, usually when I break
something else...) 

But still I would prefer the NMI watchdog not triggering then.

> So let the user know. Don't just silently say "let's kick the watchdog".

How about this approach: 

Define a new function driver_touch_watchdog() for export and it printks
something the first time it is used.

--- linux-2.6.0test2-amd64/arch/i386/kernel/nmi.c-o	2003-07-11 03:09:18.000000000 +0200
+++ linux-2.6.0test2-amd64/arch/i386/kernel/nmi.c	2003-08-06 02:03:41.000000000 +0200
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/nmi.h>
 #include <linux/sysdev.h>
+#include <linux/kallsyms.h>
 
 #include <asm/smp.h>
 #include <asm/mtrr.h>
@@ -455,3 +456,18 @@
 EXPORT_SYMBOL(enable_lapic_nmi_watchdog);
 EXPORT_SYMBOL(disable_timer_nmi_watchdog);
 EXPORT_SYMBOL(enable_timer_nmi_watchdog);
+
+/* Deprecated function to silence the NMI watchdog for long waits.
+   Better fix the driver instead of using this. */
+void driver_touch_watchdog(void)
+{ 
+	static int used; 
+	if (!used) { 
+		print_symbol(KERN_ERR "Function %s uses driver_touch_watchdog.\n",
+			     __builtin_return_address(0));
+	} 
+	used = 1;
+	touch_nmi_watchdog();
+}  
+
+EXPORT_SYMBOL(driver_touch_watchdog);

-Andi

Re: [PATCH] Export touch_nmi_watchdog

[Ingo Oeser]

On Wed, Aug 06, 2003 at 02:07:16AM +0200, Andi Kleen wrote:
> On Tue, Aug 05, 2003 at 03:14:00PM -0700, Linus Torvalds wrote:
> > 	#ifdef CONFIG_WATCHDOG
> > 	#warning This driver does bad things and will not work
> > 	#endif
> 
> Well the problem is that many of my multiple CPU testboxes have a fusion
> controller (it's the standard on board chip on the AMD Quartet and Newisys
> systems). 
 
That means, that this driver has a large (paying?) customer base.
This should be enough reason to put some efforts into fixing it.
At least it should for the (big) companies selling servers based
on this chipset, because it will help their customers a lot and
their own support staff finding latency problems.

So if you don't have the time to fix it, it's simply a problem
with your management ignoring its customers ;-/

Regards

Ingo Oeser

Re: [PATCH] Export touch_nmi_watchdog

[Linus Torvalds]

On Wed, 6 Aug 2003, Ingo Oeser wrote:
>
> So if you don't have the time to fix it, it's simply a problem
> with your management ignoring its customers ;-/

Well, probably no. The _customers_ are probably perfectly happy with just 
papering the thing over. I'm not worried about that side. So I'd be more 
worried about the developers in the long run.

		Linus