Date: Thu, 23 Oct 2014 12:14:03 -0400 From: John Baldwin <jhb@freebsd.org> To: Mateusz Guzik <mjg@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r273401 - head/sys/kern Message-ID: <201410231214.04027.jhb@freebsd.org> In-Reply-To: <201410211905.s9LJ5jDb032492@svn.freebsd.org> References: <201410211905.s9LJ5jDb032492@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201410231214.04027.jhb>