Date: Wed, 31 Aug 2011 20:15:40 +0200 From: Hans Petter Selasky <hselasky@freebsd.org> To: freebsd-arch@freebsd.org Cc: Andriy Gapon <avg@freebsd.org> Subject: Re: skipping locks, mutex_owned, usb Message-ID: <201108312015.40791.hselasky@freebsd.org> In-Reply-To: <DD9206EC-A969-434A-ABC5-15B2857C571C@bsdimp.com> References: <4E53986B.5000804@FreeBSD.org> <4E5E5DA0.4060003@FreeBSD.org> <DD9206EC-A969-434A-ABC5-15B2857C571C@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201108312015.40791.hselasky>