Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 21 Jul 2015 11:50:12 -0700
From:      John Baldwin <jhb@freebsd.org>
To:        Mateusz Guzik <mjguzik@gmail.com>
Cc:        Adrian Chadd <adrian.chadd@gmail.com>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
Subject:   Re: svn commit: r285125 - in head/sys: kern sys
Message-ID:  <3863130.vz23U50G0A@ralph.baldwin.cx>
In-Reply-To: <20150721083922.GB6736@dft-labs.eu>
References:  <201507040654.t646sGO7044196@repo.freebsd.org> <CAJ-VmokSbajFxOKPU=u4AQgPmtPqJXvzfbp4%2Bhrcd-PsCYq7PA@mail.gmail.com> <20150721083922.GB6736@dft-labs.eu>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday, July 21, 2015 10:39:22 AM Mateusz Guzik wrote:
> Cc'ing jhb@ who made relevant changes to rmlocks
> 
> On Mon, Jul 20, 2015 at 11:21:30PM -0700, Adrian Chadd wrote:
> > this happend whilst doing 'sysctl -vmstat 1' in one window, 'top' in
> > another window, and 'kldunload uhci' as root:
> > 
> > Unread portion of the kernel message buffer:
> > uhci5: detached
> > panic: sleepq_add: td 0xc75d06a0 to sleep on wchan 0xc0b3e8e4 with
> > sleeping prohibited
> >
> [..]
> 
> > #10 0xc06a882c in kassert_panic (fmt=<value optimized out>) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_shutdown.c:634
> > #11 0xc06f449b in sleepq_add (wchan=0xc0b3e8e4, wmesg=<value optimized
> > out>, flags=<value optimized out>, queue=<value optimized out>) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/subr_sleepqueue.c:308
> > #12 0xc06b167b in _sx_xlock_hard (sx=0xc0b3e8e4, tid=<value optimized
> > out>, opts=<value optimized out>, file=0x0, line=18) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_sx.c:697
> > #13 0xc06b0841 in _sx_xlock (sx=0xc0b3e8e4, opts=<value optimized
> > out>, file=0xc09f2d35
> > "/usr/home/adrian/work/freebsd/head/src/sys/kern/kern_rmlock.c",
> > line=411) at sx.h:154
> > #14 0xc06a4510 in _rm_rlock (rm=0xc0b3e8cc, tracker=0xeaa61ad0,
> > trylock=<value optimized out>) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_rmlock.c:411
> > #15 0xc06a4e2d in _rm_rlock_debug (rm=0xc0b3e8cc, tracker=0xeaa61ad0,
> > trylock=0) at /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_rmlock.c:665
> > #16 0xc06b5da4 in sysctl_root_handler_locked (oid=0xc0a6ee20,
> > arg1=<value optimized out>, arg2=<value optimized out>, req=<value
> > optimized out>, tracker=0xeaa61ad0) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_sysctl.c:170
> > #17 0xc06b5531 in sysctl_root (arg1=<value optimized out>, arg2=<value
> > optimized out>) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_sysctl.c:1692
> > #18 0xc06b5ac2 in userland_sysctl (td=<value optimized out>,
> > name=<value optimized out>, namelen=2, old=<value optimized out>,
> > oldlenp=<value optimized out>, inkernel=<value optimized out>,
> > new=<value optimized out>, newlen=<value optimized out>, retval=0x12,
> >     flags=<value optimized out>) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_sysctl.c:1797
> > #19 0xc06b5908 in sys___sysctl (uap=0xeaa61ca8) at
> > /usr/home/adrian/work/freebsd/head/src/sys/kern/kern_sysctl.c:1724
> > #20 0xc096aaee in syscall (frame=<value optimized out>) at subr_syscall.c:133
> > #21 0xc0955c5c in Xint0x80_syscall () at
> > /usr/home/adrian/work/freebsd/head/src/sys/i386/i386/exception.s:278
> 
> rmlock(9) states:
>      Sleepable read-mostly locks are created by passing RM_SLEEPABLE to
>      rm_init_flags().  Unlike normal read-mostly locks, sleepable read-mostly
>      locks follow the same lock ordering rules as sx(9) locks.  Sleepable
>      read-mostly locks do not propagate priority to writers, but they do
>      propagate priority to readers.  Writers are permitted to sleep while
>      holding a read-mostly lock, but readers are not.  Unlike other sleepable
>      locks such as sx(9) locks, readers must use try operations on other
>      sleepable locks to avoid sleeping.
> 
> May be that's my bad English, but I read that:
> rm_rlock(...);
> /* can't sleep here */
> rm_runlock(...);

That is correct.

> Turns out it's the rm_rlock itself which must not sleep and you have to
> rm_try_rlock instead.

Hmm, I think instead, the THREAD_NO_SLEEPING in rm_rlock() just needs to
be moved.  Or rather, rm_rlock_hard() needs to do THREAD_SLEEPING_OK
around the sx_xlock and then THREAD_NO_SLEEPING afterwards.  Something
like this:

Index: kern/kern_rmlock.c
===================================================================
--- kern/kern_rmlock.c  (revision 284344)
+++ kern/kern_rmlock.c  (working copy)
@@ -407,9 +407,11 @@
                                return (0);
                }
        } else {
-               if (rm->lock_object.lo_flags & LO_SLEEPABLE)
+               if (rm->lock_object.lo_flags & LO_SLEEPABLE) {
+                       THREAD_SLEEPING_OK();
                        sx_xlock(&rm->rm_lock_sx);
-               else
+                       THREAD_NO_SLEEPING();
+               } else
                        mtx_lock(&rm->rm_lock_mtx);
        }


> As for the usage here, sysctl seems like a natural consumer for the lock
> since after the kernel boots multiuser tree change is a "once in a year"
> event. This offers a very minor performance gain as well. The patch can
> be reverted back to a mere sx lock, but currently sysctl is the only
> in-tree consumer of sleepable rmlocks, so I would argue it is beneficial
> to keep it as a user if only to know the feature is somewhat
> operational.

RM_SLEEPABLE was first added as a feature for an out-of-tree consumer
(Isilon).  I merely documented it when adding WITNESS support to rmlocks
and fleshing out assertions, etc.

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3863130.vz23U50G0A>