From owner-freebsd-hackers@FreeBSD.ORG Fri Apr 27 05:53:24 2007 Return-Path: X-Original-To: freebsd-hackers@freebsd.org Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id A233516A403 for ; Fri, 27 Apr 2007 05:53:24 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe12.swip.net [212.247.155.97]) by mx1.freebsd.org (Postfix) with ESMTP id 19D5C13C46C for ; Fri, 27 Apr 2007 05:53:23 +0000 (UTC) (envelope-from hselasky@c2i.net) X-Cloudmark-Score: 0.000000 [] Received: from [193.71.38.142] (account mc467741@c2i.net HELO [10.42.11.147]) by mailfe12.swip.net (CommuniGate Pro SMTP 5.1.7) with ESMTPA id 304137636; Fri, 27 Apr 2007 07:53:22 +0200 From: Hans Petter Selasky To: Julian Elischer Date: Fri, 27 Apr 2007 07:53:05 +0200 User-Agent: KMail/1.9.5 References: <200704262136.33196.hselasky@c2i.net> <46311708.5030002@elischer.org> In-Reply-To: <46311708.5030002@elischer.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200704270753.05438.hselasky@c2i.net> Cc: freebsd-hackers@freebsd.org Subject: Re: msleep() on recursivly locked mutexes X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Apr 2007 05:53:24 -0000 On Thursday 26 April 2007 23:18, Julian Elischer wrote: > Hans Petter Selasky wrote: > > Hi, > > > > In the new USB stack I have defined the following: > > > > u_int32_t > > mtx_drop_recurse(struct mtx *mtx) > > { > > u_int32_t recurse_level = mtx->mtx_recurse; > > u_int32_t recurse_curr = recurse_level; > > > > mtx_assert(mtx, MA_OWNED); > > > > while(recurse_curr--) { > > mtx_unlock(mtx); > > } > > > > return recurse_level; > > } > > The reason that mutexes ever recurse in the first place is usually because > one piece of code calls itself (or a related piece of code) in a blind > manner.. in other words, it doesn't know it is doing so. The whole concept > of recursing mutexes is a bit gross. The concept of blindly unwinding them > is I think just asking for trouble. > > Further the idea that holding a mutex "except for when we sleep" is a > generally bright idea is also a bit odd to me.. If you hold a mutex and > release it during sleep you probably should invalidate all assumptions you > made during the period before you slept as whatever you were protecting has > possibly been raped while you slept. I have seen too many instances where > people just called msleep and dropped the mutex they held, picked it up > again on wakeup, and then blithely continued on without checking what > happened while they were asleep. > > It seems to me that if someone else held that lock for some purpose when > you slept, then you don't know what it was that they were trying to > protect, so you don't know what to recheck when you wake up.. I think this > is extremely dangerous as you don't know when you are in this situation.. > > Personally I think Matt Dillon had a good idea when he suggested that the > need to sleep when holding a lock should require the lock holder to back > out as far as needed as to be able to see why the lock was held and to > comfortably cope with the situaution when the protected structures got > frobbed while it slept. (He actually at one time committed some primitives > to do this. I forget their names and htey were never used, but the idea has > some merrit.) > > This change may be OK in the situations you plan but it makes me very > uncomfortable. it basically should have a big sign on it saying "You could > spend years looking for the wierd bugs you are likely to get if you use > this" on it. Yes, but could we factor this code out into a msleep_recurse() function then? And the mtx_pickup/mtx_drop functions I would rather see in "kern_synch.c", because it is so much faster to change the recurse level, than it is to do a while loop. --HPS