From owner-freebsd-current@FreeBSD.ORG Thu May 14 22:05:29 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1F162106567A for ; Thu, 14 May 2009 22:05:29 +0000 (UTC) (envelope-from pisymbol@gmail.com) Received: from yw-out-2324.google.com (yw-out-2324.google.com [74.125.46.28]) by mx1.freebsd.org (Postfix) with ESMTP id CA39C8FC12 for ; Thu, 14 May 2009 22:05:28 +0000 (UTC) (envelope-from pisymbol@gmail.com) Received: by yw-out-2324.google.com with SMTP id 9so886815ywe.13 for ; Thu, 14 May 2009 15:05:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=QmFcNLX/L1iRXiIUgu8pEN7fQwWQMjWX9DW8rV0EymE=; b=QgdzFD6TdxnLIUPmf+UGlbS7yeRsrVp7jvW8J3heHa8+SOXh7y6Q7W+QvAp9E9ipaX Yw34onU3wRL44rV4lPulU4+1sZ199mgqL+CI41FPPuUrnUt42wNxcHmpooSyXYwZP0nN Badf5Z/GQOp1SMs34p20ws2FaGl1f1WndCU6I= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=YbTHwbw4FqDBNnG8o2DwO78+KG8ZzpXAKlbrqqOWZli2Qr4tQg+tTo8wdShXCw5b4I FdEuOu4SOydPclnaW38+lEnEvqse7PuVwHy8clh1Wy0a0uypSIL9nMhfbadh+5nWg+qa LnrmKFWpCKtYLwn/RccjpN9itcYPs0X+s2ZCs= MIME-Version: 1.0 Received: by 10.100.215.17 with SMTP id n17mr3758102ang.87.1242338727983; Thu, 14 May 2009 15:05:27 -0700 (PDT) In-Reply-To: <4A0C8F30.8080404@delphij.net> References: <3c0b01820905141202w113966dp4bfbab73d84d585@mail.gmail.com> <4A0C7544.6010304@delphij.net> <3c0b01820905141301h1b08fc0ay1e6a1676b5a149d4@mail.gmail.com> <4A0C8F30.8080404@delphij.net> Date: Thu, 14 May 2009 18:05:27 -0400 Message-ID: <3c0b01820905141505r5b8ea64bq42cdea70ee288015@mail.gmail.com> From: Alexander Sack To: d@delphij.net Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@freebsd.org Subject: Re: Broadcom bge(4) panics while shutting down X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 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: Thu, 14 May 2009 22:05:29 -0000 On Thu, May 14, 2009 at 5:37 PM, Xin LI wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi, Alexander, > > Alexander Sack wrote: > [...] >>>> Preliminary testing shows no more panics start and stopping ports >>>> under heavy load (panics were almost immediate otherwise). >>>> >>>> Thoughts? >>> I think this would solve the problem but I'm not sure whether this woul= d >>> increase some overhead on the RX path. =A0It seems that there is a race >>> between bge_release_resources() and bge_intr(), I mean, it might be a >>> good idea to "drain" bge_intr() instead? >> >> Are you talking about detach time? =A0Because bge_stop() gets called >> before bge_release_resources() and stops host interrupts so where is >> the race again? =A0I mean at this point no more interrupts should be >> delivered to bge_intr() (I can confirm from spec since BGE has >> released it in the wild). =A0So why would you "drain" it at this >> point....(the hardware is down including the firmware). >> >> I agree it adds a little overhead to the standard bge_rxeof() path >> which I agree is very sensitive to change. =A0However, I think the check >> at top is tolerable since the other recourse is crash. =A0I mean its >> very easy to reproduce. =A0Flood a Broadcom card with traffic then stop >> the card and let the race begin...it will go down in bge_rxeof() after >> bge_stop releases the lock. >> >> I actually did not look at changing anything structurally to perhaps >> make this whole predicament better but minimally there should be a >> shield against this no? > > I'm all for a workaround at this point, but I really want to make sure > that we are doing things in a safer manner if we can reach it timely. > > What about something like this, in my opinion, because bge_rxeof() would > unlock sc in the middle, it would be probably better to re-check the > IFF_DRV_RUNNING flag and avoid calling txeof after that (I've moved the > condition to after we re-grab the lock to match the rest of the code: > > =3D=3D=3D=3D=3D > [delphij@charlie] /sys/dev/bge> svn diff -x -p > Index: if_bge.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > - --- if_bge.c =A0(revision 191995) > +++ if_bge.c =A0 =A0(working copy) > @@ -3073,7 +3073,7 @@ bge_rxeof(struct bge_softc *sc) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bus_dmamap_sync(sc->bge_cdata.bge_rx_jumbo= _ring_tag, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sc->bge_cdata.bge_rx_jumbo_ring_ma= p, BUS_DMASYNC_POSTREAD); > > - - =A0 =A0 while(sc->bge_rx_saved_considx !=3D > + =A0 =A0 =A0 while (sc->bge_rx_saved_considx !=3D > =A0 =A0 =A0 =A0 =A0 =A0sc->bge_ldata.bge_status_block->bge_idx[0].bge_rx_= prod_idx) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct bge_rx_bd =A0 =A0 =A0 =A0*cur_rx; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uint32_t =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rx= idx; > @@ -3193,6 +3193,9 @@ bge_rxeof(struct bge_softc *sc) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BGE_UNLOCK(sc); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(*ifp->if_input)(ifp, m); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BGE_LOCK(sc); > + > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return; > =A0 =A0 =A0 =A0} Xin this looks fine by me, I actually put this up in the while loop as I mentioned before which I think is functionality equivalent (can you gain some optimizations by putting in the while loop though compiler wise than a separate compilation unit?). > =A0 =A0 =A0 =A0if (stdcnt > 0) > @@ -3301,6 +3304,10 @@ bge_poll(struct ifnet *ifp, enum poll_cmd cmd, int > > =A0 =A0 =A0 =A0sc->rxcycles =3D count; > =A0 =A0 =A0 =A0bge_rxeof(sc); > + =A0 =A0 =A0 if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BGE_UNLOCK(sc); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return; > + =A0 =A0 =A0 } So here if bge_rxeof() drops the lock, bge_stop() sneaks in reset flag and doesn't bother to process txeof(). I can buy that. > =A0 =A0 =A0 =A0bge_txeof(sc); > =A0 =A0 =A0 =A0if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bge_start_locked(ifp); > @@ -3370,7 +3377,9 @@ bge_intr(void *xsc) > =A0 =A0 =A0 =A0if (ifp->if_drv_flags & IFF_DRV_RUNNING) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Check RX return ring producer/consumer.= */ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bge_rxeof(sc); > + =A0 =A0 =A0 } > > + =A0 =A0 =A0 if (ifp->if_drv_flags & IFF_DRV_RUNNING) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Check TX ring producer/consumer. */ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bge_txeof(sc); > =A0 =A0 =A0 =A0} Same here, avoid txeof() if bge_rxeof()/bge_stop() reset the DRV_RUNNING flag. Luckily txeof() doesn't drop the lock so no check is needed in there. I'm happier....should have thought about poll though (I hate polling). -aps