From owner-freebsd-arch@FreeBSD.ORG Thu Aug 25 13:33:12 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 64E0D106564A; Thu, 25 Aug 2011 13:33:12 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 822558FC0C; Thu, 25 Aug 2011 13:33:11 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id QAA05164; Thu, 25 Aug 2011 16:33:09 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <4E564F15.3010809@FreeBSD.org> Date: Thu, 25 Aug 2011 16:33:09 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:5.0) Gecko/20110705 Thunderbird/5.0 MIME-Version: 1.0 To: John Baldwin References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org> In-Reply-To: <201108230911.09021.jhb@freebsd.org> X-Enigmail-Version: 1.2pre Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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:33:12 -0000 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. -- Andriy Gapon