cvs commit: src/sys/dev/md md.c

[Don Lewis]

On 16 Sep, Nate Lawson wrote:
> Pawel Jakub Dawidek wrote:
>> On Thu, Sep 16, 2004 at 12:40:23PM -0700, Nate Lawson wrote:
>> +> >@@ -379,9 +379,8 @@
>> +> > 	bp->bio_bcount = bp->bio_length;
>> +> > 	mtx_lock(&sc->queue_mtx);
>> +> > 	bioq_disksort(&sc->bio_queue, bp);
>> +> >-	mtx_unlock(&sc->queue_mtx);
>> +> >-
>> +> > 	wakeup(sc);
>> +> >+	mtx_unlock(&sc->queue_mtx);
>> +> > }
>> +> 
>> +> I think the original order is correct since you can occur 2 switches if 
>> +> you wakeup first and then unlock.

I think that is is only true of there is an idle CPU and
ADAPTIVE_MUTEXES are not enabled.  If there is not an idle CPU, the
other thread won't start running for a while.  If ADAPTIVE_MUTEXES are
enabled and there is an idle CPU, the other thread will start executing,
but it will spin for a a short period of time until the mutex is
unlocked.

>> Nope, this order was wrong:
>> 
>> thread1		thread2
>> -----------------------
>> mtx_lock(mtx)
>> ...
>> mtx_unlock(mtx)
>> 		mtx_lock(mtx)
>> wakeup(ptr)
>> 		msleep(ptr, mtx) <- Race, it will be never woken up.
> 
> You still have a race, like this:
> 
> thread1               thread2
> -----------------------------
> mtx_lock(mtx)
> wakeup(ptr)
> mtx_unlock(mtx)
>                        mtx_lock(mtx)
>                        msleep(ptr, mtx)
> 
> You should be checking the work condition in thread 2 while holding the 
> mutex but before going to sleep.  Adding work to the queue happens in 
> thread 1 where you write "..." and that is done with the mutex held so 
> there is no race.  The full diagram with this detail included is:
> 
> thread1               thread2
> -----------------------------
> mtx_lock(mtx)
> add work to queue
> mtx_unlock(mtx)
>                        mtx_lock(mtx)
> wakeup(ptr)
>                        check queue for work item
>                        if (!work item)
>                            msleep(ptr, mtx)
>                        else
>                            dequeue work item and loop
> 
> Since the work item is added in thread1 with the mutex held, the check 
> for it in thread2 is safe and race-free.  A wakeup is only there to 
> kickstart thread2 if it's asleep.  If it's running, it needs to check 
> atomically that there is no work before sleeping.  If it doesn't do 
> this, it's a bug.

What about the situation where you know that it is unlikely that there
is another thread sleeping on ptr?   Thread 2 would set a flag
(protected by mtx) recording that it wants to be awakened, and thread 1
would only call wakeup() if the flag was set.  In this case thread 1
would have to hold the mutex until after the wakeup(), otherwise there
would be a race on the wakeup wanted flag.

How expensive is an unnecessary wakeup()?