From owner-freebsd-current@FreeBSD.ORG Sun Jul 18 02:29:06 2004 Return-Path: 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 5B5BC16A4CE; Sun, 18 Jul 2004 02:29:06 +0000 (GMT) Received: from harmony.village.org (rover.village.org [168.103.84.182]) by mx1.FreeBSD.org (Postfix) with ESMTP id A2D6543D31; Sun, 18 Jul 2004 02:29:05 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from localhost (warner@rover2.village.org [10.0.0.1]) by harmony.village.org (8.12.11/8.12.11) with ESMTP id i6I2RLKO005811; Sat, 17 Jul 2004 20:27:21 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Sat, 17 Jul 2004 20:27:29 -0600 (MDT) Message-Id: <20040717.202729.12970058.imp@bsdimp.com> To: green@freebsd.org From: "M. Warner Losh" In-Reply-To: <20040717191943.GU1626@green.homeunix.org> References: <20040717182841.GR1626@green.homeunix.org> <20040717.124803.33210527.imp@bsdimp.com> <20040717191943.GU1626@green.homeunix.org> X-Mailer: Mew version 3.3 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit cc: current@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: Sun, 18 Jul 2004 02:29:06 -0000 In message: <20040717191943.GU1626@green.homeunix.org> Brian Fundakowski Feldman writes: : 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. I'd also like to appologize for getting my nose out of joint. I have some personal issues hitting finality in my life right now, and I've noticed I have a thinner skin of late as a result. : 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. One thing that I could do, if we *KNOW* that the bridge ISR is run first of all the shared interrupts, is to use atomic ops to clear/set the OK flag. That would prevent me from calling the ISR, while pushing down into kern_intr.c the details of dealing with the issues of inserting/removing from the list. This would also get the priority right, which I currently don't do right in the pccbb code. I'd then register a wrapper for these interrupts that checks the OK flag and neglects to call them if it isn't OK. This does add a little overhead, but not all that much, to the ISR. And it would ensure that pccard/cardbus interrupts have the same semantics as others in the system. The question is do we know this? And are there plans that the ISRs might run on different CPUs in parallel... I also did some research. I likely didn't see this because I grab Giant when attaching/detaching cards and the ISR used to be marked as needing Giant, so the race you've found wouldn't happened. When I removed the Giant flag from the bridge ISR, this race reared its ugly head. : 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. I'm not sure what I think of this, so I'll think about it for a while and reply later. Warner