Date: Fri, 17 Sep 2004 21:25:26 -0400 From: John Baldwin <jhb@FreeBSD.org> To: Don Lewis <truckman@FreeBSD.org> Cc: wollman@khavrinen.lcs.mit.edu Subject: Re: cvs commit: src/sys/dev/md md.c Message-ID: <200409172125.26106.jhb@FreeBSD.org> In-Reply-To: <200409170437.i8H4btEo062532@gw.catspoiler.org> References: <200409170437.i8H4btEo062532@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve" = http://www.FreeBSD.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200409172125.26106.jhb>