From owner-svn-src-all@FreeBSD.ORG Thu Oct 23 18:38:24 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id AF2BBD58; Thu, 23 Oct 2014 18:38:24 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 89C6AD3A; Thu, 23 Oct 2014 18:38:24 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 7F604B9B1; Thu, 23 Oct 2014 14:38:23 -0400 (EDT) From: John Baldwin To: Mateusz Guzik Subject: Re: svn commit: r273401 - head/sys/kern Date: Thu, 23 Oct 2014 12:14:03 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <201410211905.s9LJ5jDb032492@svn.freebsd.org> In-Reply-To: <201410211905.s9LJ5jDb032492@svn.freebsd.org> MIME-Version: 1.0 Message-Id: <201410231214.04027.jhb@freebsd.org> Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 23 Oct 2014 14:38:23 -0400 (EDT) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Oct 2014 18:38:24 -0000 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