From owner-freebsd-current@FreeBSD.ORG  Sat Jul 17 19:33:51 2004
Return-Path: <owner-freebsd-current@FreeBSD.ORG>
Delivered-To: freebsd-current@freebsd.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP
	id 49D2E16A4CE; Sat, 17 Jul 2004 19:33:51 +0000 (GMT)
Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21])
	by mx1.FreeBSD.org (Postfix) with ESMTP
	id 418B143D4C; Sat, 17 Jul 2004 19:33:51 +0000 (GMT)
	(envelope-from bmilekic@FreeBSD.org)
Received: from freefall.freebsd.org (bmilekic@localhost [127.0.0.1])
	i6HJXpa4085280;	Sat, 17 Jul 2004 19:33:51 GMT
	(envelope-from bmilekic@freefall.freebsd.org)
Received: (from bmilekic@localhost)
	by freefall.freebsd.org (8.12.11/8.12.11/Submit) id i6HJXpq8085279;
	Sat, 17 Jul 2004 19:33:51 GMT
	(envelope-from bmilekic)
Date: Sat, 17 Jul 2004 19:33:51 +0000
From: Bosko Milekic <bmilekic@FreeBSD.org>
To: Brian Fundakowski Feldman <green@freebsd.org>
Message-ID: <20040717193351.GA83772@freefall.freebsd.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.4.1i
cc: current@freebsd.org
cc: "M. Warner Losh" <imp@bsdimp.com>
Subject: Re: pccbb crashes when detaching (unsafe interrupt handler)
X-BeenThere: freebsd-current@freebsd.org
X-Mailman-Version: 2.1.1
Precedence: list
List-Id: Discussions about the use of FreeBSD-current
	<freebsd-current.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-current>,
	<mailto:freebsd-current-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-current>
List-Post: <mailto:freebsd-current@freebsd.org>
List-Help: <mailto:freebsd-current-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-current>,
	<mailto:freebsd-current-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 17 Jul 2004 19:33:51 -0000


Brian Feldman wrote:
>>From currentish kern_intr.c:
>                     if ((ih->ih_flags & IH_DEAD) != 0) {
>                             mtx_lock(&ithd->it_lock);
>                             TAILQ_REMOVE(&ithd->it_handlers, ih,
>                               ih_next);
>                             wakeup(ih);
>                             mtx_unlock(&ithd->it_lock);
>                             goto restart;
>                     }
>We add a flag IH_PIN:
>                     if ((ih->ih_flags & (IH_DEAD | IH_PIN)) != 0) {
>                             if ((ih->ih_flags & IH_DEAD) == 0) {
>                                     wakeup(ih);
>                                     continue;
>                             }
>                             mtx_lock(&ithd->it_lock);
>                             TAILQ_REMOVE(&ithd->it_handlers,
>                               ih, ih_next);
>                             wakeup(ih);
>                             mtx_unlock(&ithd->it_lock);
>                             goto restart;
>                     }

    Neither -current nor your version should be holding the ithd lock
    across the wakeup().  What is being done is that the ithd lock which
    is supposed to protect the ithd data structure is being used to
    also protect against a temporary (insignificant) race in wakeup().
    However, the race is non-fatal and should exist.  The race that
    results from trying to plug this one is now between the thread
    doing the wakeup returning from the wakeup and dropping the lock
    before the woken-up thread picks it up.  This is (IMO) more wrong
    than the non-fatal race this is trying to plug against.

    I'll comment more on your proposed solution later but thought I
    should get the above off my chest, because it's not the first time
    I see it.

-Bosko