Date: Wed, 31 Aug 2011 10:32:57 -0600 From: Warner Losh <imp@bsdimp.com> To: Andriy Gapon <avg@freebsd.org> Cc: Hans Petter Selasky <hselasky@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: skipping locks, mutex_owned, usb Message-ID: <DD9206EC-A969-434A-ABC5-15B2857C571C@bsdimp.com> In-Reply-To: <4E5E5DA0.4060003@FreeBSD.org> References: <4E53986B.5000804@FreeBSD.org> <4E5A099F.4060903@FreeBSD.org> <201108281127.44696.hselasky@freebsd.org> <201108310943.24476.jhb@freebsd.org> <4E5E5DA0.4060003@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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() >>>>> { >>>>>=20 >>>>> if (!mtx_owned(&Giant)) { >>>>> =09 >>>>> mtx_lock(&Giant); >>>>> =09 >>>>> func(); >>>>> mtx_unlock(&Giant); >>>>> =09 >>>>> return; >>>>> =09 >>>>> } >>>>> =09 >>>>> // etc ... >>>>>=20 >>>>> } >>>>=20 >>>> Ohhh, nothing seems simple with the USB code: >>>>=20 >>>> /* make sure that the BUS mutex is not locked */ >>>> drop_bus =3D 0; >>>> while (mtx_owned(&xroot->udev->bus->bus_mtx)) { >>>> mtx_unlock(&xroot->udev->bus->bus_mtx); >>>> drop_bus++; >>>> } >>>>=20 >>>> /* make sure that the transfer mutex is not locked */ >>>> drop_xfer =3D 0; >>>> while (mtx_owned(xroot->xfer_mtx)) { >>>> mtx_unlock(xroot->xfer_mtx); >>>> drop_xfer++; >>>> } >>>>=20 >>>> So many unconventional tricks. >>>=20 >>> Similar code is used in the DROP_GIANT and PICKUP_GIANT macros. You = might=20 >> want=20 >>> to check all references to mtx_owned() in the kernel, and make a set = of=20 >>> exceptions for post-panic code execution. >>=20 >> Giant is special because it is a hack to let us run non-MPSAFE code. = Other >> locks should not follow its model. >>=20 >=20 > Hans, >=20 > 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 >=20 > So why the mutex unwinding/rewinding code is present at all? > What kind of situations is it supposed to prevent? >=20 > 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. Warner=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?DD9206EC-A969-434A-ABC5-15B2857C571C>