svn commit: r273401 - head/sys/kern

[John Baldwin]

On Tuesday, October 21, 2014 3:05:45 pm Mateusz Guzik wrote:
> Author: mjg
> Date: Tue Oct 21 19:05:44 2014
> New Revision: 273401
> URL: https://svnweb.freebsd.org/changeset/base/273401
> 
> Log:
>   Implement shared locking for sysctl.

A bit more detail in this message was warranted.  We had some shared locking 
in the past (r192125) but it was subsequently reverted (r216060).

In particular, explaining how you fixed the part of 216060 that caused the
shared locking to be reverted would have answered Bjoern's earlier question as 
well.  It's important that log messages not only explain what they are doing, 
but why they are doing so.  (For exmaple, it would have been nice if 216060 
had explained why it was reverting the shared locking in its log message, but 
it didn't. :( )

I think that you fixed the issues by a combination of using sysctl_lock/unlock 
to handle shared vs exclusive locking for the caller and you used atomic ops 
on the running count (previously the xlock allowed use of non-atomic ops on 
the running count).

sysctl_root() is now only called with a shared lock held, so you should change 
its assertion accordingly.  sysctl_register_oid() is still called with the 
xlock held, so you can't remove the sysctl_lock() stuff from 
sysctl_root_handler_locked() entirely.  OTOH, there is a stale comment in 
kern_sysctl.c about having a public sysctl_lock/unlock API that you can just 
remove.  Also, given that sysctl_lock/unlock are only used in 
sysctl_root_handler_locked(), I would probably remove them and just inline 
them in the one place they are needed.

Finally, getting pre-commit review is fairly easy these days with phabricator 
and would allow you to have had all these things noted and addressed before it 
went into the tree.

-- 
John Baldwin