Re: stray irq's in systat

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
_at__at_ -663,13 +637,40 _at__at_
 	    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)
_at__at_ -677,4 +678,9 _at__at_

 	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)
_at__at_ -685,5 +691,4 _at__at_
 	}

-	update_intrname(irq, name);
 	return (0);
 }
%%%

Bruce