From owner-freebsd-hackers@FreeBSD.ORG Thu Apr 26 21:17:49 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 3ECFD16A401 for ; Thu, 26 Apr 2007 21:17:49 +0000 (UTC) (envelope-from julian@elischer.org) Received: from outJ.internet-mail-service.net (outJ.internet-mail-service.net [216.240.47.233]) by mx1.freebsd.org (Postfix) with ESMTP id 2C66B13C455 for ; Thu, 26 Apr 2007 21:17:49 +0000 (UTC) (envelope-from julian@elischer.org) Received: from mx0.idiom.com (HELO idiom.com) (216.240.32.160) by out.internet-mail-service.net (qpsmtpd/0.32) with ESMTP; Thu, 26 Apr 2007 13:44:58 -0700 Received: from julian-mac.elischer.org (nat.ironport.com [63.251.108.100]) by idiom.com (Postfix) with ESMTP id 7EEA8125B23; Thu, 26 Apr 2007 14:17:48 -0700 (PDT) Message-ID: <46311708.5030002@elischer.org> Date: Thu, 26 Apr 2007 14:18:00 -0700 From: Julian Elischer User-Agent: Thunderbird 2.0.0.0 (Macintosh/20070326) MIME-Version: 1.0 To: Hans Petter Selasky References: <200704262136.33196.hselasky@c2i.net> In-Reply-To: <200704262136.33196.hselasky@c2i.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Thu, 26 Apr 2007 21:17:49 -0000 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. > > void > mtx_pickup_recurse(struct mtx *mtx, u_int32_t recurse_level) > { > mtx_assert(mtx, MA_OWNED); > > while(recurse_level--) { > mtx_lock(mtx); > } > return; > } > > When I do a msleep() I do it like this: > > level = mtx_drop_recurse(ctd->p_mtx); > > error = msleep(ctd, ctd->p_mtx, 0, > "config td sleep", timeout); > > mtx_pickup_recurse(ctd->p_mtx, level); > > Are there any comments on integrating this functionality into msleep(), and > adding mtx_drop_recurse() and mtx_pickup_recurse() to the FreeBSD kernel? > > --HPS > _______________________________________________ > freebsd-hackers@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-hackers > To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"