From owner-freebsd-arch@FreeBSD.ORG Wed Aug 31 18:18:35 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 B57FD106567B; Wed, 31 Aug 2011 18:18:35 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe04.c2i.net [212.247.154.98]) by mx1.freebsd.org (Postfix) with ESMTP id 09CC08FC19; Wed, 31 Aug 2011 18:18:34 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.1 cv=nSp/lo0FDChGKjONaW+LFp6gLzUQbrH1VOzHQZWpGes= c=1 sm=1 a=ni9ZY7ZFbhgA:10 a=WQU8e4WWZSUA:10 a=8nJEP1OIZ-IA:10 a=CL8lFSKtTFcA:10 a=i9M/sDlu2rpZ9XS819oYzg==:17 a=-G6q7kbFCyEwIIUnyiIA:9 a=H_8iCRKF1ZwUWQpn-5AA:7 a=wPNLvfGTeEIA:10 a=i9M/sDlu2rpZ9XS819oYzg==:117 Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe04.swip.net (CommuniGate Pro SMTP 5.2.19) with ESMTPA id 173083654; Wed, 31 Aug 2011 20:18:29 +0200 To: freebsd-arch@freebsd.org From: Hans Petter Selasky X-Face: *nPdTl_}RuAI6^PVpA02T?$%Xa^>@hE0uyUIoiha$pC:9TVgl.Oq,NwSZ4V" =?iso-8859-1?q?=7CLR=2E+tj=7Dg5=0A=09=25V?=,x^qOs~mnU3]Gn; cQLv&.N>TrxmSFf+p6(30a/{)KUU!s}w\IhQBj}[g}bj0I3^glmC( =?iso-8859-1?q?=0A=09=3AAuzV9=3A=2EhESm-x4h240C=609=3Dw?= Date: Wed, 31 Aug 2011 20:15:54 +0200 MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201108312015.54587.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: Wed, 31 Aug 2011 18:18:35 -0000 On Wednesday 31 August 2011 18:32:57 Warner Losh wrote: > On Aug 31, 2011, at 10:13 AM, Andriy Gapon wrote: > > on 31/08/2011 16:43 John Baldwin said the following: > >> On Sunday, August 28, 2011 5:27:44 am Hans Petter Selasky wrote: > >>> On Sunday 28 August 2011 11:25:51 Andriy Gapon wrote: > >>>> on 23/08/2011 15:09 Andriy Gapon said the following: > >>>>> 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 ... > >>>>> > >>>>> } > >>>> > >>>> Ohhh, nothing seems simple with the USB code: > >>>> > >>>> /* make sure that the BUS mutex is not locked */ > >>>> drop_bus = 0; > >>>> while (mtx_owned(&xroot->udev->bus->bus_mtx)) { > >>>> > >>>> mtx_unlock(&xroot->udev->bus->bus_mtx); > >>>> drop_bus++; > >>>> > >>>> } > >>>> > >>>> /* make sure that the transfer mutex is not locked */ > >>>> drop_xfer = 0; > >>>> while (mtx_owned(xroot->xfer_mtx)) { > >>>> > >>>> mtx_unlock(xroot->xfer_mtx); > >>>> drop_xfer++; > >>>> > >>>> } > >>>> > >>>> So many unconventional tricks. > >>> > >>> Similar code is used in the DROP_GIANT and PICKUP_GIANT macros. You > >>> might > >> > >> want > >> > >>> to check all references to mtx_owned() in the kernel, and make a set of > >>> exceptions for post-panic code execution. > >> > >> Giant is special because it is a hack to let us run non-MPSAFE code. > >> Other locks should not follow its model. > > > > Hans, > > > > actually this makes me wonder... It seems that usbd_transfer_poll() > > function that utilizes the above code is called from a number of > > xxx_poll routines in various usb drivers (ukbd, umass and a bunch of > > serial drivers): > > $ glimpse -l usbd_transfer_poll | fgrep .c > > /usr/src/sys/dev/usb/input/ukbd.c > > /usr/src/sys/dev/usb/usb_transfer.c > > /usr/src/sys/dev/usb/storage/umass.c > > /usr/src/sys/dev/usb/serial/ucycom.c > > /usr/src/sys/dev/usb/serial/umoscom.c > > /usr/src/sys/dev/usb/serial/umcs.c > > /usr/src/sys/dev/usb/serial/umodem.c > > /usr/src/sys/dev/usb/serial/uchcom.c > > /usr/src/sys/dev/usb/serial/uipaq.c > > /usr/src/sys/dev/usb/serial/ugensa.c > > /usr/src/sys/dev/usb/serial/uark.c > > /usr/src/sys/dev/usb/serial/ufoma.c > > /usr/src/sys/dev/usb/serial/umct.c > > /usr/src/sys/dev/usb/serial/ubsa.c > > /usr/src/sys/dev/usb/serial/uslcom.c > > /usr/src/sys/dev/usb/serial/uplcom.c > > /usr/src/sys/dev/usb/serial/uvscom.c > > /usr/src/sys/dev/usb/serial/uftdi.c > > /usr/src/sys/dev/usb/serial/ubser.c > > > > So why the mutex unwinding/rewinding code is present at all? > > What kind of situations is it supposed to prevent? > > > > Personally I think that we either know that those drivers should not hold > > the locks in question (bus_mtx and xfer_mtx) or we know that they hold > > them. I do not see why we have to be conditional here or have a loop > > even... > > Today, I think we know these things. In the past, as the code was written, > there was a lot more special case code needed for giant cohabitation. What Warner says is correct. The code was written when SCHEDULER_STOPPED() was not implemented. Really the usbd_transfer_poll should simply return if SCHEDULER_STOPPED() is not set. And then those mtx_unlock()/mtx_lock() cases can simply be removed. --HPS