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>