Date: Thu, 25 Aug 2011 12:35:15 +0200 From: Hans Petter Selasky <hselasky@c2i.net> To: freebsd-arch@freebsd.org Cc: Andriy Gapon <avg@freebsd.org> Subject: Re: skipping locks, mutex_owned, usb Message-ID: <201108251235.15853.hselasky@c2i.net> In-Reply-To: <201108230911.09021.jhb@freebsd.org> References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 23 August 2011 15:11:08 John Baldwin wrote:
> > I. [Why] do we need this pattern?
> > Can the code be re-written in a smarter (if not to say proper) way?
>
Hi,
> 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:
Unless Witness is changed, that won't work. It will lead to LOR warnings I
think.
Imagine that the root function locks Giant, then another lock is locked and
then ukbd_poll() is called. Won't the second lock of Giant cause a LOR
warning?
>
> 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);
> }
>
> Most code should not be abusing mtx_owned() in this fashion. For Giant you
> should just follow a simple pattern like above that lets it recurse. For
> your own locks you generally should use things like mtx_assert() to
> require all callers of a given routine to hold the lock rather than
> recursively acquiring the lock. Very few legitimate cases of mtx_owned()
> exist IMO. It is debatable if we should even have a mtx_owned() routine
> since we have mtx_assert().
How would you solve the LOR case?
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201108251235.15853.hselasky>