From owner-svn-src-all@FreeBSD.ORG Sun Oct 26 01:57:49 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8E5D918D; Sun, 26 Oct 2014 01:57:49 +0000 (UTC) Received: from mail-wi0-x229.google.com (mail-wi0-x229.google.com [IPv6:2a00:1450:400c:c05::229]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 88EC085B; Sun, 26 Oct 2014 01:57:48 +0000 (UTC) Received: by mail-wi0-f169.google.com with SMTP id q5so3640091wiv.4 for ; Sat, 25 Oct 2014 18:57:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=gX7VvKB62yHOXwxNN+qbvMdM7Z6HvpOFLbPRtRhjAno=; b=kW+AqYZMXvKr9us1mzEYaVJpi4s6hU0tDiBa+00jJMC7RI0ycDQSpkkHih2oh8tCkv 5AD3JhBFirWVOvw4qAT1TjYrvjoOof0XNs3ho5rZsqIIVnAw+VrbP/iFsujQwhu1LPUD BtDow/V7RgA62RHrx1vWFyjLpFeCPQgtFEDRihp3Qd53/Ce+9030iZuarrFrfcQG71Bg NazCDLr+YY5jUEkSwZ0tFnnoZuCdEOjtFn3YIuTrwvCtXyz+9R+jswcKB+rV1nytcABs lrNa6H5bqor3F4UenIz4cYjdmrPmre0irPaHxCyzGXdu/lsa5BYUUFcMYRxuG7JM00bG NtrQ== X-Received: by 10.194.81.6 with SMTP id v6mr13863769wjx.39.1414288666905; Sat, 25 Oct 2014 18:57:46 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id ic4sm6723470wid.19.2014.10.25.18.57.45 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 25 Oct 2014 18:57:45 -0700 (PDT) Date: Sun, 26 Oct 2014 02:57:43 +0100 From: Mateusz Guzik To: John Baldwin Subject: Re: svn commit: r273401 - head/sys/kern Message-ID: <20141026015743.GE19066@dft-labs.eu> References: <201410211905.s9LJ5jDb032492@svn.freebsd.org> <201410231214.04027.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201410231214.04027.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik 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: Sun, 26 Oct 2014 01:57:49 -0000 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