Date: Wed, 31 Aug 2011 19:13:20 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org>, Hans Petter Selasky <hselasky@FreeBSD.org> Cc: freebsd-arch@FreeBSD.org Subject: Re: skipping locks, mutex_owned, usb Message-ID: <4E5E5DA0.4060003@FreeBSD.org> In-Reply-To: <201108310943.24476.jhb@freebsd.org> References: <4E53986B.5000804@FreeBSD.org> <4E5A099F.4060903@FreeBSD.org> <201108281127.44696.hselasky@freebsd.org> <201108310943.24476.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E5E5DA0.4060003>