Date: Sun, 26 Oct 2014 02:57:43 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: svn commit: r273401 - head/sys/kern Message-ID: <20141026015743.GE19066@dft-labs.eu> In-Reply-To: <201410231214.04027.jhb@freebsd.org> References: <201410211905.s9LJ5jDb032492@svn.freebsd.org> <201410231214.04027.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Oct 23, 2014 at 12:14:03PM -0400, John Baldwin wrote: > 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). > I agree that commit message is quite lacking in detail, in retrospect I should have written a thing or two. So yes, in short mutual exclusion between threads disabling sysctls and ones going through them is retained. And accuracy of the counter is ensured with the use of atomic ops. > 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. I addressed stuff in https://svnweb.freebsd.org/changeset/base/273654 , although the comment needed altering and not removing. I specifically added sysctl_lock/unlock so that next folk with similar usecse will not have to. So I'm not going to remove it, but if you really don't like it feel free to do it. > > 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. > yeah, I actually do ask for review more often than not. :) -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141026015743.GE19066>