Date: Thu, 14 May 2009 19:03:22 -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: <3c0b01820905141603g59e439c1y7202532bc1f4f87@mail.gmail.com> In-Reply-To: <4A0CA096.4020706@delphij.net> References: <3c0b01820905141202w113966dp4bfbab73d84d585@mail.gmail.com> <4A0C7544.6010304@delphij.net> <3c0b01820905141301h1b08fc0ay1e6a1676b5a149d4@mail.gmail.com> <4A0C8F30.8080404@delphij.net> <3c0b01820905141505r5b8ea64bq42cdea70ee288015@mail.gmail.com> <4A0CA096.4020706@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 14, 2009 at 6:52 PM, Xin LI <delphij@delphij.net> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi, Alexander, > > Alexander Sack wrote: > [...] >>> @@ -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?). > > I think the two is not semantically the same... =A0For this case an > explicit 'return' would mean that no further actions, say the things > right after the while loop, would be taken. =A0In my opinion that this is > better since there is no protection over these DMA maps (which could > have been released elsewhere). Oh snap, I had a brain fart. Ugh. You are correct Xin. Oh man, I even said in my first post I want to AVOID touching the DMA maps and/or hardware registers (notice the access in the jumbo case) as soon as IFF_DRV_RUNNING gets reset. I like the explicit return much better that's why I had it that way but as John pointed out, tis better to do it where you put it which is right after reacquiring the lock. > Note that I'm still not quite confident about the logic, we might just > narrowed but not closed the race completely - these > BGE_LOCK()/UNLOCK()'s could hit some problem if one thread has done > bge_stop() very quickly. =A0That would require more work, though, I don't > have a very good plan at this moment... This has been a constant problem that I've seen in the labs. I've seen this with polling as well as tuning the number of RING descriptors (there is a thread I had a while back ago about this when I was dealing with BPF drops and bge(4)). I think the patch as it stands though will suffice for the majority of cases and again I thank you and John for addressing it in short-order. -aps
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3c0b01820905141603g59e439c1y7202532bc1f4f87>