From owner-freebsd-arch@FreeBSD.ORG Thu Aug 25 13:45:47 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 4572A106564A; Thu, 25 Aug 2011 13:45:47 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id F2A4B8FC16; Thu, 25 Aug 2011 13:45:46 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 9D4CB46B53; Thu, 25 Aug 2011 09:45:46 -0400 (EDT) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id B482A8A037; Thu, 25 Aug 2011 09:45:45 -0400 (EDT) From: John Baldwin To: Andriy Gapon Date: Thu, 25 Aug 2011 09:45:24 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110617; KDE/4.5.5; amd64; ; ) References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org> <4E564F15.3010809@FreeBSD.org> In-Reply-To: <4E564F15.3010809@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201108250945.24606.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Thu, 25 Aug 2011 09:45:46 -0400 (EDT) Cc: freebsd-arch@freebsd.org 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 13:45:47 -0000 On Thursday, August 25, 2011 9:33:09 am Andriy Gapon wrote: > on 23/08/2011 16:11 John Baldwin said the following: > > On Tuesday, August 23, 2011 8:09:15 am Andriy Gapon wrote: > >> > >> Yes, the subject sounds quite hairy, so please let me try to explain it. > >> First, let's consider one concrete function: > >> > >> static int > >> ukbd_poll(keyboard_t *kbd, int on) > >> { > >> struct ukbd_softc *sc = kbd->kb_data; > >> > >> if (!mtx_owned(&Giant)) { > >> /* XXX cludge */ > >> int retval; > >> mtx_lock(&Giant); > >> retval = ukbd_poll(kbd, on); > >> mtx_unlock(&Giant); > >> return (retval); > >> } > >> > >> 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 */ > >> } > >> return (0); > >> } > >> > >> This "XXX cludge" [sic] pattern is scattered around a few functions in the ukbd > >> code and perhaps other usb code: > >> func() > >> { > >> if (!mtx_owned(&Giant)) { > >> mtx_lock(&Giant); > >> func(); > >> mtx_unlock(&Giant); > >> return; > >> } > >> > >> // etc ... > >> } > >> > >> I have a few question directly and indirectly related to this pattern. > >> > >> I. [Why] do we need this pattern? > >> Can the code be re-written in a smarter (if not to say proper) way? > > > > 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: > > > > 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); > > } > > > > Thank you for clarifying this, I agree this is simpler than the original code and > should work exactly the same. > > There is more trouble with a few other functions that actually behave differently > (even if slightly) depending on what mtx_owned(&Giant) returns. Not sure if > that's a legal use or an antipattern. > > For example: ukbd_ioctl, ukbd_check, ukbd_check_char, ukbd_read, ukbd_read_char. I think many of these can be fixed the same way. The one issue I see so far is when things like ukbd_check() just fail if it is not polling and Giant is not held. You need to find out if that is due to similar misunderstandings about Giant LORs or not. If it is, you can probably just always acquire Giant. -- John Baldwin