Date: Thu, 14 May 2009 18:05:27 -0400 From: Alexander Sack <pisymbol@gmail.com> To: d@delphij.net Cc: freebsd-current@freebsd.org Subject: Re: Broadcom bge(4) panics while shutting down Message-ID: <3c0b01820905141505r5b8ea64bq42cdea70ee288015@mail.gmail.com> In-Reply-To: <4A0C8F30.8080404@delphij.net> References: <3c0b01820905141202w113966dp4bfbab73d84d585@mail.gmail.com> <4A0C7544.6010304@delphij.net> <3c0b01820905141301h1b08fc0ay1e6a1676b5a149d4@mail.gmail.com> <4A0C8F30.8080404@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 14, 2009 at 5:37 PM, Xin LI <delphij@delphij.net> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3c0b01820905141505r5b8ea64bq42cdea70ee288015>