On 27-Dec-00 Jonathan Chen wrote:
> On Thu, Dec 14, 2000 at 12:47:51PM -0800, John Baldwin wrote:
>> In other news, while wandering through the cardbus code, I discovered that
>> pccbb softc's have an internal mutex much to my surprise, and that they
>> weren't
>> quite being used properly AFAICT.  In the pccb0 kthread, they were
>> acquired/released without Giant.  However, in the rest of the pccbb driver,
>> they are acquired/released with Giant.  This leads to lock order violations 
>> which could potentially deadlock the machine at some point.  Also, the
>> pccbb0
>> kthread holds the mutex across the entire card insertion and removal events,
>> which is not quite right.  Mutexes should only be held for short periods of
>> time.  As such, I've futzed around with the mutex operations in the pccbb
>> driver some.  It may not be completely correct, but I think it is an
>> improvement.  The patch for this can be found at
>> http://www.FreeBSD.org/~jhb/patches/pccbb.patch.
> [I just now recalled your mentioning this a while back, so here's my much
> delayed response]
> Originally I had write the code with spl's, and at one point decided to
> convert them to mutex after finding spl's don't do anything.  However, I
> don't claim to know a whole lot about mutexes or FreeBSD's implementation
> of it.  Perhaps someone can point me to the Great Definitive Source?

There isn't one yet.  We are still getting some of the internals done to make
mutexes in drivers useful. :)  For now all drivers still run under Giant.

> The first thing I'm unclear about is the deal with Giant.  My understanding
> is that Giant is something one would acquire if he wanted to be really
> exclusive, or something like that.  John - you mentioned I was
> acquiring/releasing with/without Giant - but I don't see how I'm doing
> things differently in and out of the kthread.  The only place I mentioned
> Giant was right before kthread_exit, and that's necessary least
> kthread_exit calls panic() on me.  On your patch, you acquire Giant at the
> start of the kthread - wouldn't that hold onto the lock until the thread
> exits, and make other things attempting to lock Giant block?

Right now Giant is gotten on nearly every entry into the kernel (fast interrupt
handlers don't grab it and INTR_MPSAFE interrupt threads don't either.) 
Basically, as a mutex it protects everything in the kernel.  Thus, it is only
safe to run w/o holding Giant if _all_ of the data structures you are touching
are protected somehow.  In the kthread, you didn't acquire Giant, so all of the
data structures it touched (for example, in other driver attach and probes)
weren't protected at all.  Thus, cpu A could be running the pccbb thread doing
an attach, and cpu B could be doing something else on the same driver and they
would happily corrupt each other, crash teh system, etc.  When your thread
sleeps, it gives Giant up, so it won't block the system indefinitely.  (Giant
is special with regards to tsleep(9) in that case.)  Other kthreads grab Giant
when they first start up, see the aio daemon code for example.

> As for the 'holding onto mutex for a long time' problem, I'm not sure if
> your solution is best.  I originally put in the locks firstly because
> that's how NetBSD had it, and secondly so that card removal interrupts
> while card is still attaching wouldn't be lost.  Your patched version
> allows for wakeup() to be called while insertion/removal is being
> processed.  I've glanced through the code and didn't see anything that
> requires mutual exclusivity between the kthread the the interrupt handler,
> so perhaps a better solution would be to simply not use mutex at all?

I was mostly trying to push the locks down so that they weren't held during
attach and remove.  I was using them to protect mostly the kthread state flag
in the softc, as well as other variables in the softc.

> Comments?


John Baldwin <[EMAIL PROTECTED]> -- http://www.FreeBSD.org/~jhb/
PGP Key: http://www.baldwin.cx/~john/pgpkey.asc
"Power Users Use the Power to Serve!"  -  http://www.FreeBSD.org/

To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Reply via email to