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>