Re: stray irq's in systat

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
> _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
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"
>