From owner-cvs-src@FreeBSD.ORG Tue Apr 29 11:44:29 2003 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id CFC3237B401 for ; Tue, 29 Apr 2003 11:44:29 -0700 (PDT) Received: from mail.speakeasy.net (mail15.speakeasy.net [216.254.0.215]) by mx1.FreeBSD.org (Postfix) with ESMTP id E0CEA43FCB for ; Tue, 29 Apr 2003 11:44:27 -0700 (PDT) (envelope-from jhb@FreeBSD.org) Received: (qmail 10356 invoked from network); 29 Apr 2003 18:44:33 -0000 Received: from unknown (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender )encrypted SMTP for ; 29 Apr 2003 18:44:33 -0000 Received: from laptop.baldwin.cx (gw1.twc.weather.com [216.133.140.1]) by server.baldwin.cx (8.12.8/8.12.8) with ESMTP id h3TIiPOv019079; Tue, 29 Apr 2003 14:44:25 -0400 (EDT) (envelope-from jhb@FreeBSD.org) Message-ID: X-Mailer: XFMail 1.5.4 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: <16046.49785.35005.126940@grasshopper.cs.duke.edu> Date: Tue, 29 Apr 2003 14:44:30 -0400 (EDT) From: John Baldwin To: Andrew Gallatin cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: "M. Warner Losh" cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Apr 2003 18:44:30 -0000 On 29-Apr-2003 Andrew Gallatin wrote: > > M. Warner Losh writes: > > In message: <20030429133708.A84234@grasshopper.cs.duke.edu> > > Andrew Gallatin writes: > > : What is this deadlock with bus_teardown_intr? Could we possibly fix > > : this some other way than by adding (mostly) useless code to the > > : critical path? > > > > I can remove it if it really annoys you that much. The locking code > > itself will swamp the extra load/compare that happens. It will put > > > "A million here, a million there, pretty soon it adds up" > > I'm just disappointed to see things bloat, and bloat, and bloat more. > FreeBSD used to be fast.. > > > > the race back into the code, however. This likely means that some > > higher level of locking is necessary so that we can make sure that the > > interrupts can't happen once detach starts. > > > > Warner > > > How about just replacing the intr handler with a dummy? That seems > fairly easy and doesn't require pessimizing critical path code. > > Would you mind trying something like the following (sorry, I'm in the > middle of moving and don't have machines handy to test ... most of my > lab is packed). I don't even have a tree handy to do diffs. > > Drew > > > Add this to kern/kern_intr.c: > > void > ithread_orphan_handler(void *arg) > { > } > > int > ithread_replace_handler(void *cookie, driver_intr_t func) > { > struct intrhand *handler = (struct intrhand *)cookie; > struct ithd *ithread; >#ifdef INVARIANTS > struct intrhand *ih; >#endif > ithread = handler->ih_ithread; > KASSERT(ithread != NULL, > ("interrupt handler \"%s\" has a NULL interrupt thread", > handler->ih_name)); > > if (handler == NULL) > return (EINVAL); > > mtx_lock(&ithread->it_lock); > > handler->ih_handler = func; > mtx_unlock(&ithread->it_lock); > return (0); > } > > > And make if_fxp.c look like this: > > /* > * Detach interface. > */ > static int > fxp_detach(device_t dev) > { > struct fxp_softc *sc = device_get_softc(dev); > int s; > > FXP_LOCK(sc); > s = splimp(); > > sc->gone = 1; > /* > * Close down routes etc. > */ > ether_ifdetach(&sc->arpcom.ac_if); > > /* > * Stop DMA and drop transmit queue, but disable interruptsfirst. > */ > CSR_WRITE_1(sc, FXP_CSR_SCB_INTRCNTL, FXP_SCB_INTR_DISABLE); > fxp_stop(sc); > if(ithread_replace_handler(sc->ih, ithread_orphan_handler)) > panic("ithread_replace_handler didn't"); > FXP_UNLOCK(sc); > > /* > * Unhook interrupt before dropping lock. This is to prevent > * races with fxp_intr(). > */ > bus_teardown_intr(sc->dev, sc->irq, sc->ih); > sc->ih = NULL; > > splx(s); > > /* Release our allocated resources. */ > fxp_release(sc); > return (0); > } > > static void > fxp_intr(void *xsc) > { > struct fxp_softc *sc = xsc; > struct ifnet *ifp = &sc->sc_if; > u_int8_t statack; > > FXP_LOCK(sc); >#ifdef DEVICE_POLLING > if (ifp->if_flags & IFF_POLLING) { > <..> First off, it's a gross API violation. ithread's are supposed to be transparent (except that you can use locks in your interrupt handlers). Secondly, you still have the same race as if you just removed the gone condition. We don't hold the ithread lock while executing handlers, so there is nothing preventing the handler from being executed on another CPU concurrently with your detach function. In fact, it could easily be blocked on the FXP lock. You do your magic pointer foo, then unlock the fxp. The unlock releases the interrupt handler on another CPU which happily executes _after_ the completion of bus_teardown_intr() which has all sorts of bad failure modes, like panicing when it tries to FXP_UNLOCK at the end because back on the first CPU you've done a mtx_destroy(). -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/