From owner-p4-projects@FreeBSD.ORG Thu Nov 2 17:07:03 2006 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 4A71916A47C; Thu, 2 Nov 2006 17:07:03 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0BAD316A40F; Thu, 2 Nov 2006 17:07:03 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.FreeBSD.org (Postfix) with ESMTP id 782DB43D53; Thu, 2 Nov 2006 17:07:02 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from zion.baldwin.cx (zion.baldwin.cx [192.168.0.7]) (authenticated bits=0) by server.baldwin.cx (8.13.6/8.13.6) with ESMTP id kA2H6qRg013455; Thu, 2 Nov 2006 12:06:56 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: Hans Petter Selasky Date: Thu, 2 Nov 2006 11:51:19 -0500 User-Agent: KMail/1.9.1 References: <200611010112.kA11C1Jt066210@repoman.freebsd.org> <200611011047.07627.jhb@freebsd.org> <200611021222.48108.hselasky@c2i.net> In-Reply-To: <200611021222.48108.hselasky@c2i.net> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200611021151.19396.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [192.168.0.1]); Thu, 02 Nov 2006 12:06:56 -0500 (EST) X-Virus-Scanned: ClamAV 0.88.3/2149/Thu Nov 2 10:27:15 2006 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: Perforce Change Reviews , Scott Long Subject: Re: PERFORCE change 108878 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Nov 2006 17:07:03 -0000 On Thursday 02 November 2006 06:22, Hans Petter Selasky wrote: > On Wednesday 01 November 2006 16:47, John Baldwin wrote: > > On Tuesday 31 October 2006 20:12, Scott Long wrote: > > > http://perforce.freebsd.org/chv.cgi?CH=108878 > > > > > > Change 108878 by scottl@scottl-x64 on 2006/11/01 01:11:30 > > > > > > For some wonderful reason, you cannot pass &Giant to msleep. Work > > > around that in a crude fashion. Also add some more assertions. > > > > Ah, yes, that would be most unhappy. I guess mostly the idea is that Giant > > should be rather implicit and explicitly using Giant for an object lock is > > discouraged. I'm not sure that is what you are doing though. I think > > maybe you are borrowing Giant that's already held? > > I use this patch: > > /* preliminary fix for a bug in msleep on FreeBSD, > * which cannot sleep with Giant: > */ > #define msleep(i,m,p,w,t) msleep(i,(((m) == &Giant) ? NULL : (m)),p,w,t) > > Really this issue should be fixed. It happens just because GIANT_DROP is done > too early in "msleep()". Giant is special in msleep() and friends to make sure it is first in the lock order, but unlock doesn't matter for lock order, and actually, the current order is less intuitive. I think it's the way it is now because we inherited it from BSD/OS. Also in theory old code under Giant should be using tsleep() and not msleep() anyway. It actually won't hurt to move DROP_GIANT later though. How about this: Index: kern_synch.c =================================================================== RCS file: /host/cvs/usr/cvs/src/sys/kern/kern_synch.c,v retrieving revision 1.281 diff -u -r1.281 kern_synch.c --- kern_synch.c 15 Jun 2006 06:41:57 -0000 1.281 +++ kern_synch.c 2 Nov 2006 16:47:27 -0000 @@ -172,12 +172,12 @@ CTR5(KTR_PROC, "msleep: thread %p (pid %ld, %s) on %s (%p)", (void *)td, (long)p->p_pid, p->p_comm, wmesg, ident); - DROP_GIANT(); if (mtx != NULL) { mtx_assert(mtx, MA_OWNED | MA_NOTRECURSED); WITNESS_SAVE(&mtx->mtx_object, mtx); mtx_unlock(mtx); } + DROP_GIANT(); /* * We put ourselves on the sleep queue and start our timeout Index: kern_condvar.c =================================================================== RCS file: /host/cvs/usr/cvs/src/sys/kern/kern_condvar.c,v retrieving revision 1.55 diff -u -r1.55 kern_condvar.c --- kern_condvar.c 23 Feb 2006 00:13:58 -0000 1.55 +++ kern_condvar.c 2 Nov 2006 16:48:05 -0000 @@ -146,8 +146,8 @@ sleepq_lock(cvp); cvp->cv_waiters++; - DROP_GIANT(); mtx_unlock(mp); + DROP_GIANT(); sleepq_add(cvp, mp, cvp->cv_description, SLEEPQ_CONDVAR); sleepq_wait(cvp); @@ -193,8 +193,8 @@ sleepq_lock(cvp); cvp->cv_waiters++; - DROP_GIANT(); mtx_unlock(mp); + DROP_GIANT(); sleepq_add(cvp, mp, cvp->cv_description, SLEEPQ_CONDVAR | SLEEPQ_INTERRUPTIBLE); @@ -247,8 +247,8 @@ sleepq_lock(cvp); cvp->cv_waiters++; - DROP_GIANT(); mtx_unlock(mp); + DROP_GIANT(); sleepq_add(cvp, mp, cvp->cv_description, SLEEPQ_CONDVAR); sleepq_set_timeout(cvp, timo); @@ -304,8 +304,8 @@ sleepq_lock(cvp); cvp->cv_waiters++; - DROP_GIANT(); mtx_unlock(mp); + DROP_GIANT(); sleepq_add(cvp, mp, cvp->cv_description, SLEEPQ_CONDVAR | SLEEPQ_INTERRUPTIBLE); -- John Baldwin