svn commit: r285125 - in head/sys: kern sys

[Mateusz Guzik]

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(...);

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

Now, why does not rm_rlock panic outright when passed a sleepable lock?

Further, that's a rather unfortunate property. With only skimming
through rmlock implementation, I get why you would want to actualy not
sleep in rm_rlock.

So how about a helper (say rm_rlock_sleepable) which would be roughly:
void
_rm_rlock_sleepable(struct rmlock *rm, struct rm_priotracker *tracker)
{

       MPASS(rm->lock_object.lo_flags & LO_SLEEPABLE);
       while (rm_try_rlock(rm, tracker) == 0) {
               sx_slock(&rm->rm_lock_sx);
               sx_sunlock(&rm->rm_lock_sx);
       }
}

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.

-- 
Mateusz Guzik <mjguzik gmail.com>