Date: Thu, 25 Aug 2011 09:45:24 -0400 From: John Baldwin <jhb@freebsd.org> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-arch@freebsd.org Subject: Re: skipping locks, mutex_owned, usb Message-ID: <201108250945.24606.jhb@freebsd.org> In-Reply-To: <4E564F15.3010809@FreeBSD.org> References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org> <4E564F15.3010809@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, August 25, 2011 9:33:09 am Andriy Gapon wrote:
> on 23/08/2011 16:11 John Baldwin said the following:
> > On Tuesday, August 23, 2011 8:09:15 am Andriy Gapon wrote:
> >>
> >> Yes, the subject sounds quite hairy, so please let me try to explain it.
> >> First, let's consider one concrete function:
> >>
> >> static int
> >> ukbd_poll(keyboard_t *kbd, int on)
> >> {
> >> struct ukbd_softc *sc = kbd->kb_data;
> >>
> >> if (!mtx_owned(&Giant)) {
> >> /* XXX cludge */
> >> int retval;
> >> mtx_lock(&Giant);
> >> retval = ukbd_poll(kbd, on);
> >> mtx_unlock(&Giant);
> >> return (retval);
> >> }
> >>
> >> if (on) {
> >> sc->sc_flags |= UKBD_FLAG_POLLING;
> >> sc->sc_poll_thread = curthread;
> >> } else {
> >> sc->sc_flags &= ~UKBD_FLAG_POLLING;
> >> ukbd_start_timer(sc); /* start timer */
> >> }
> >> return (0);
> >> }
> >>
> >> 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 ...
> >> }
> >>
> >> I have a few question directly and indirectly related to this pattern.
> >>
> >> I. [Why] do we need this pattern?
> >> Can the code be re-written in a smarter (if not to say proper) way?
> >
> > Giant is recursive, it should just be always acquired. Also, this recursive
> > call pattern is very non-obvious. A far more straightforward approach would
> > be to just have:
> >
> > static int
> > ukbd_poll(keyboard_t *kbd, int on)
> > {
> > struct ukbd_softc *sc = kbd->kb_data;
> >
> > mtx_lock(&Giant);
> > if (on) {
> > sc->sc_flags |= UKBD_FLAG_POLLING;
> > sc->sc_poll_thread = curthread;
> > } else {
> > sc->sc_flags &= ~UKBD_FLAG_POLLING;
> > ukbd_start_timer(sc); /* start timer */
> > }
> > mtx_unlock(&Giant);
> > return (0);
> > }
> >
>
> Thank you for clarifying this, I agree this is simpler than the original code and
> should work exactly the same.
>
> There is more trouble with a few other functions that actually behave differently
> (even if slightly) depending on what mtx_owned(&Giant) returns. Not sure if
> that's a legal use or an antipattern.
>
> For example: ukbd_ioctl, ukbd_check, ukbd_check_char, ukbd_read, ukbd_read_char.
I think many of these can be fixed the same way. The one issue I see so far
is when things like ukbd_check() just fail if it is not polling and Giant is
not held. You need to find out if that is due to similar misunderstandings
about Giant LORs or not. If it is, you can probably just always acquire
Giant.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201108250945.24606.jhb>