From owner-cvs-all@FreeBSD.ORG Sat Sep 18 02:36:08 2004 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id A941516A4D3 for ; Sat, 18 Sep 2004 02:36:08 +0000 (GMT) Received: from mail2.speakeasy.net (mail2.speakeasy.net [216.254.0.202]) by mx1.FreeBSD.org (Postfix) with ESMTP id 3946643D49 for ; Sat, 18 Sep 2004 02:36:08 +0000 (GMT) (envelope-from jhb@FreeBSD.org) Received: (qmail 8978 invoked from network); 18 Sep 2004 02:36:08 -0000 Received: from dsl027-160-063.atl1.dsl.speakeasy.net (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender ) encrypted SMTP for ; 18 Sep 2004 02:36:07 -0000 Received: from slimer.baldwin.cx (slimer.baldwin.cx [192.168.0.16]) (authenticated bits=0) by server.baldwin.cx (8.12.11/8.12.11) with ESMTP id i8I2a0OJ004409; Fri, 17 Sep 2004 22:36:04 -0400 (EDT) (envelope-from jhb@FreeBSD.org) From: John Baldwin To: Don Lewis Date: Fri, 17 Sep 2004 21:25:26 -0400 User-Agent: KMail/1.6.2 References: <200409170437.i8H4btEo062532@gw.catspoiler.org> In-Reply-To: <200409170437.i8H4btEo062532@gw.catspoiler.org> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200409172125.26106.jhb@FreeBSD.org> X-Spam-Checker-Version: SpamAssassin 2.63 (2004-01-11) on server.baldwin.cx cc: cvs-src@FreeBSD.org cc: nate@root.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org cc: wollman@khavrinen.lcs.mit.edu Subject: Re: cvs commit: src/sys/dev/md md.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 Sep 2004 02:36:08 -0000 On Friday 17 September 2004 12:37 am, Don Lewis wrote: > On 16 Sep, Garrett Wollman wrote: > > < 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 <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org