Date: Thu, 16 Sep 2004 21:37:55 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: wollman@khavrinen.lcs.mit.edu Cc: nate@root.org Subject: Re: cvs commit: src/sys/dev/md md.c Message-ID: <200409170437.i8H4btEo062532@gw.catspoiler.org> In-Reply-To: <200409162225.i8GMPFFs010481@khavrinen.lcs.mit.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
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?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200409170437.i8H4btEo062532>