From owner-freebsd-current@FreeBSD.ORG Sat Jul 17 19:19:45 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from green.homeunix.org (freefall.freebsd.org [216.136.204.21]) by hub.freebsd.org (Postfix) with ESMTP id 5484016A4D0; Sat, 17 Jul 2004 19:19:45 +0000 (GMT) Received: from green.homeunix.org (green@localhost [127.0.0.1]) by green.homeunix.org (8.12.11/8.12.11) with ESMTP id i6HJJinY093376; Sat, 17 Jul 2004 15:19:44 -0400 (EDT) (envelope-from green@green.homeunix.org) Received: (from green@localhost) by green.homeunix.org (8.12.11/8.12.11/Submit) id i6HJJipg093375; Sat, 17 Jul 2004 15:19:44 -0400 (EDT) (envelope-from green) Date: Sat, 17 Jul 2004 15:19:43 -0400 From: Brian Fundakowski Feldman To: "M. Warner Losh" Message-ID: <20040717191943.GU1626@green.homeunix.org> References: <20040717054014.GP1626@green.homeunix.org> <20040717.121712.83689795.imp@bsdimp.com> <20040717182841.GR1626@green.homeunix.org> <20040717.124803.33210527.imp@bsdimp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20040717.124803.33210527.imp@bsdimp.com> User-Agent: Mutt/1.5.6i cc: current@FreeBSD.ORG cc: jhb@FreeBSD.ORG 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 17 Jul 2004 19:19:45 -0000 On Sat, Jul 17, 2004 at 12:48:03PM -0600, M. Warner Losh wrote: > In message: <20040717182841.GR1626@green.homeunix.org> > : FreeBSD 5.x is not based on the spl > : model anymore, and unless you want to ADD the ability to do something like > : that, which is certainly quite feasible (that is, add a "blocking" count > : to the interrupt handler, which the ithread has to ack before you can > : return with the interrupt "blocked") there will be a specific cost inside > : the pccbb driver to do things safely and it will involve some sort of > : synchronization. > > I don't assume that things are spl based, I just had never seen the > race that you found. I don't want to use sx locks to solve this > because I'd one day like to make the cbb isr a fast interrupt handler > so that we can restore fast interrupt handlers to pccard and cardbus > modems. sx locks sleep, and can't be used in a fast interrupt > handler. I'd love it if we could tone down the anger and incredulity. From this point on, let's start off assuming each one of us knows what the other is talking about. None of us likes feeling insulted or slighted, so I would like to publically apologize for being provoked and taking an angry tone when I am simply interested in fixing this matter (and many others) so that 5.3 Does Not Suck. Okay, using an sx_xlock() (instead of sx_tryxlock()) would certainly prevent that, but I can understand not wanting to add more weight to the fast path. It's something no one wants, even if it's the cleanest solution. So now that I've had more time to think about it, here's my idea on the feasibility of an ithread handler critical section. It should cost no more in the fast path than it does now, other than increasing the amount of code jumped past by several instructions when it's skipped. >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; } The implementation for the caller would look almost the same as the ithread_remove_handler() function. It would push essentially all of the cost out to the code actually modifying the specifics of the interrupt handler, and should provide a sufficient critical section for any such drivers that want to create their own interrupt sub-handlers. So ithread_pin_handler()/ithread_unpin_handler() seem like a reasonable solution, if a little bit ugly to be calling ithread support code from drivers directly? I don't know if there's any reason on FreeBSD the driver should not be able to know for certain that all interrupts are registered as an intrhand on on ithread. -- Brian Fundakowski Feldman \'[ FreeBSD ]''''''''''\ <> green@FreeBSD.org \ The Power to Serve! \ Opinions expressed are my own. \,,,,,,,,,,,,,,,,,,,,,,\