Date: Wed, 27 Aug 2003 02:50:37 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Mike Jakubik <mikej@trigger.net> Cc: current@freebsd.org Subject: Re: stray irq's in systat Message-ID: <20030827022049.L914@gamplex.bde.org> In-Reply-To: <JCEIKJMCANNPGKFKGLKLAECPDKAA.mikej@trigger.net> References: <JCEIKJMCANNPGKFKGLKLAECPDKAA.mikej@trigger.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 26 Aug 2003, Mike Jakubik wrote:
> When running 'systat -vmstat 1' on FreeBSD 5.1-CURRENT #1: Mon Aug 25
> 14:54:14 EDT 2003, the interrupts section shows irq's 0 and 6 as stray. I
> remember this would happen on 4.x when I took out lpt drivers from the
> kernel, and didn't disable lpt in the bios. This however is not the case,
> and start irq's do not show up under 4.8 on this box. everything is working
> ok, I am just curious why they are showing up.
This is caused by a race calling update_intrname().
Most of the patch consists of comments about unfixed races. There are many
other bugs in this area.
%%%
Index: intr_machdep.c
===================================================================
RCS file: /home/ncvs/src/sys/i386/isa/intr_machdep.c,v
retrieving revision 1.73
diff -u -2 -r1.73 intr_machdep.c
--- intr_machdep.c 20 Oct 2002 18:02:46 -0000 1.73
+++ intr_machdep.c 6 Apr 2003 10:14:13 -0000
@@ -663,13 +637,40 @@
ithread_priority(flags), flags, cookiep);
- if ((flags & INTR_FAST) == 0 || errcode)
+ if ((flags & INTR_FAST) == 0 || errcode) {
/*
* The interrupt process must be in place, but
* not necessarily schedulable, before we
- * initialize the ICU, since it may cause an
+ * "initialize" the ICU, since "initializing" it may "cause" an
* immediate interrupt.
+ *
+ * The pointer to the interrupt counter (which is changed if
+ * the name is changed) must be in place for the same reason.
+ * Otherwise, we could and did get normal interrupts bogusly
+ * counted as stray ones, which mainly messed up systat(8)'s
+ * layout.
+ *
+ * XXX we depend on the interrupt being set up not already
+ * being enabled here. This is part of the API, and the
+ * locking for it is hopefully adequate. However, the
+ * locking is inadequate for other interrupts being set
+ * up concrrently (we race in update_intrname()) and for
+ * spurious interrupts (update_intrname() and icu_setup()
+ * need a common lock).
+ *
+ * XXX icu_unset() is only called from isa_defaultirq(), so
+ * I don't see how bus_teardown_intr() can work. I think
+ * it leaves a garbage pointer to the interrupt handler.
+ * In the non-fast case, the pointer is to sched_ithd() so
+ * which cannot be unloaded, so the only damage is that we
+ * waste time checking for errors that shouldn't happen.
+ * In the fast case, the pointer may be into an unloaded
+ * module. Presumably the interrupt is masked in another
+ * way, else we would have more problems. However, spurious
+ * interrupts can't be masked in the ICU.
*/
+ update_intrname(irq, name);
if (icu_setup(irq, sched_ithd, arg, flags) != 0)
panic("inthand_add: Can't initialize ICU");
+ }
if (errcode)
@@ -677,4 +678,9 @@
if (flags & INTR_FAST) {
+ /*
+ * XXX this clause repeats a lot of code, apparently just to
+ * vary the handler and to avoid panicing if icu_setup() fails.
+ */
+ update_intrname(irq, name);
errcode = icu_setup(irq, handler, arg, flags);
if (errcode && bootverbose)
@@ -685,5 +691,4 @@
}
- update_intrname(irq, name);
return (0);
}
%%%
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030827022049.L914>