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

[Scott Long]

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.
>>
>> 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.
> 

Or just use a semaphore.

Scott