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>