Export touch_nmi_watchdog

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
@@ -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);

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

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

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.

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

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

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

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

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.

How about this approach:

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

@@ -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

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

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

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

Because they're a hard to ignore reality?

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.

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

-Andi

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

-Andi