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>