From owner-freebsd-arch@FreeBSD.ORG Thu Aug 25 10:37:56 2011 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9106E1065670; Thu, 25 Aug 2011 10:37:56 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe06.c2i.net [212.247.154.162]) by mx1.freebsd.org (Postfix) with ESMTP id ACFBD8FC08; Thu, 25 Aug 2011 10:37:55 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.1 cv=RCcoR6dhO4r2plSBAg1vtKNWl/yGWcPYGWoK9vAZDcU= c=1 sm=1 a=SvYTsOw2Z4kA:10 a=ni9ZY7ZFbhgA:10 a=WQU8e4WWZSUA:10 a=8nJEP1OIZ-IA:10 a=CL8lFSKtTFcA:10 a=i9M/sDlu2rpZ9XS819oYzg==:17 a=Z3Q73qxEUil-ZZdhi3kA:9 a=lmfJouIePbVyiHv-sIwA:7 a=wPNLvfGTeEIA:10 a=lu8jnEJfmK6Ue0LV:21 a=hvKIPJzv4vDNgfuC:21 a=i9M/sDlu2rpZ9XS819oYzg==:117 Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe06.swip.net (CommuniGate Pro SMTP 5.2.19) with ESMTPA id 169798292; Thu, 25 Aug 2011 12:37:46 +0200 From: Hans Petter Selasky To: freebsd-arch@freebsd.org Date: Thu, 25 Aug 2011 12:35:15 +0200 User-Agent: KMail/1.13.5 (FreeBSD/8.2-STABLE; KDE/4.4.5; amd64; ; ) References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org> In-Reply-To: <201108230911.09021.jhb@freebsd.org> X-Face: *nPdTl_}RuAI6^PVpA02T?$%Xa^>@hE0uyUIoiha$pC:9TVgl.Oq, NwSZ4V"|LR.+tj}g5 %V,x^qOs~mnU3]Gn; cQLv&.N>TrxmSFf+p6(30a/{)KUU!s}w\IhQBj}[g}bj0I3^glmC( :AuzV9:.hESm-x4h240C`9=w MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201108251235.15853.hselasky@c2i.net> Cc: Andriy Gapon Subject: Re: skipping locks, mutex_owned, usb X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Aug 2011 10:37:56 -0000 On Tuesday 23 August 2011 15:11:08 John Baldwin wrote: > > I. [Why] do we need this pattern? > > Can the code be re-written in a smarter (if not to say proper) way? > Hi, > Giant is recursive, it should just be always acquired. Also, this > recursive call pattern is very non-obvious. A far more straightforward > approach would be to just have: Unless Witness is changed, that won't work. It will lead to LOR warnings I think. Imagine that the root function locks Giant, then another lock is locked and then ukbd_poll() is called. Won't the second lock of Giant cause a LOR warning? > > static int > ukbd_poll(keyboard_t *kbd, int on) > { > struct ukbd_softc *sc = kbd->kb_data; > > mtx_lock(&Giant); > if (on) { > sc->sc_flags |= UKBD_FLAG_POLLING; > sc->sc_poll_thread = curthread; > } else { > sc->sc_flags &= ~UKBD_FLAG_POLLING; > ukbd_start_timer(sc); /* start timer */ > } > mtx_unlock(&Giant); > return (0); > } > > Most code should not be abusing mtx_owned() in this fashion. For Giant you > should just follow a simple pattern like above that lets it recurse. For > your own locks you generally should use things like mtx_assert() to > require all callers of a given routine to hold the lock rather than > recursively acquiring the lock. Very few legitimate cases of mtx_owned() > exist IMO. It is debatable if we should even have a mtx_owned() routine > since we have mtx_assert(). How would you solve the LOR case? --HPS