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

[John Baldwin]

On Friday 17 September 2004 12:37 am, Don Lewis wrote:
> On 16 Sep, Garrett Wollman wrote:
> > <<On Thu, 16 Sep 2004 15:15:15 -0700, Nate Lawson <nate at root.org> said:
> >> 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:
> >
> > Of course, getting this right is complicated enough that we have an
> > entire abstraction to assist.
> >
> >> 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
> >
> > mtx_lock(mtx)
> > add work to queue
> > cv_signal(worktodo)
> > mtx_unlock(mtx)
> > 			mtx_lock(mtx)
> > 			for (;;) {
> > 				check queue for work item
> > 				if (!work item)
> > 					cv_wait(cv, mtx)
> > 				else {
> > 					dequeue work item
> > 					do work
> > 				}
> > 			}
> > 			mtx_unlock(mtx)
>
> It looks to me like there is a race condition in the cv_wait()
> implementation.
>
>         			cvp->cv_waiters++;
>         			DROP_GIANT();
>         			mtx_unlock(mp);
> mtx_lock()
> ...
> if (cvp->cv_waiters > 0) {
> 	cvp->cv_waiters--;
> 	sleepq_signal();
> }
>         			sleepq_add(...);
>         			sleepq_wait(cvp);
>
>
> Also, doesn't this potentially have the same problem with extra context
> switches that Nate mentioned earlier?

Currently cv's do require that the mutex be held across wakeup.  I do plan to 
remove that requirement in 6.x and the cv_waiters optimization along with it 
(or maybe protect the waiter count with the sleepqueue chain mutex instead, 
but the whole point of cv_waiters was to avoid the sleepqueues completely in 
the first place).  The removal of said requirement has just been low on the 
priority list.  msleep/wakeup do not have any such requirement currently.

-- 
John Baldwin <jhb at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org