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);
Andi Kleen <[email protected]> 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?
On Tue, Aug 05, 2003 at 12:38:11PM -0700, Andrew Morton wrote:
> Andi Kleen <[email protected]> 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
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 <[email protected]> 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
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
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.
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
> 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
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
> 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
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% :)
> > > - 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
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
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
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
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
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