Date: Thu, 2 Nov 2006 11:51:19 -0500 From: John Baldwin <jhb@freebsd.org> To: Hans Petter Selasky <hselasky@c2i.net> Cc: Perforce Change Reviews <perforce@freebsd.org>, Scott Long <scottl@freebsd.org> Subject: Re: PERFORCE change 108878 for review Message-ID: <200611021151.19396.jhb@freebsd.org> In-Reply-To: <200611021222.48108.hselasky@c2i.net> References: <200611010112.kA11C1Jt066210@repoman.freebsd.org> <200611011047.07627.jhb@freebsd.org> <200611021222.48108.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200611021151.19396.jhb>