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

[Nate Lawson]

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.

-- 
Nate