Date: Tue, 26 Aug 2003 09:57:15 -0700 (PDT) From: Julian Elischer <julian@elischer.org> To: Bruce Evans <bde@zeta.org.au> Cc: current@freebsd.org Subject: Re: stray irq's in systat Message-ID: <Pine.BSF.4.21.0308260957040.33736-100000@InterJet.elischer.org> In-Reply-To: <20030827022049.L914@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
any plans to commit this?
On Wed, 27 Aug 2003, Bruce Evans wrote:
> 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
> _______________________________________________
> freebsd-current@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0308260957040.33736-100000>