From owner-freebsd-arch@FreeBSD.ORG Wed Aug 31 16:13:25 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 A2D551065670; Wed, 31 Aug 2011 16:13:25 +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 57EC28FC12; Wed, 31 Aug 2011 16:13:23 +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 TAA22758; Wed, 31 Aug 2011 19:13:21 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <4E5E5DA0.4060003@FreeBSD.org> Date: Wed, 31 Aug 2011 19:13:20 +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 , Hans Petter Selasky References: <4E53986B.5000804@FreeBSD.org> <4E5A099F.4060903@FreeBSD.org> <201108281127.44696.hselasky@freebsd.org> <201108310943.24476.jhb@freebsd.org> In-Reply-To: <201108310943.24476.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: Wed, 31 Aug 2011 16:13:25 -0000 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... -- Andriy Gapon