From owner-svn-src-all@freebsd.org Sun Feb 5 15:17:47 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 800F2CD17CF; Sun, 5 Feb 2017 15:17:47 +0000 (UTC) (envelope-from danfe@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2610:1c1:1:6074::16:84]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "freefall.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 60DD41366; Sun, 5 Feb 2017 15:17:47 +0000 (UTC) (envelope-from danfe@freebsd.org) Received: by freefall.freebsd.org (Postfix, from userid 1033) id B26E01DC1; Sun, 5 Feb 2017 15:17:46 +0000 (UTC) Date: Sun, 5 Feb 2017 15:17:46 +0000 From: Alexey Dokuchaev To: Mateusz Guzik Cc: Steven Hartland , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik Subject: Re: svn commit: r313260 - head/sys/kern Message-ID: <20170205151746.GA6841@FreeBSD.org> References: <201702050140.v151eRXX090326@repo.freebsd.org> <42c790bb-df12-2a50-6181-24bac5c72d34@multiplay.co.uk> <20170205030006.GB4375@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170205030006.GB4375@dft-labs.eu> User-Agent: Mutt/1.7.1 (2016-10-04) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 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, 05 Feb 2017 15:17:47 -0000 On Sun, Feb 05, 2017 at 04:00:06AM +0100, Mateusz Guzik wrote: > For instance, plugging an unused variable, a memory leak, doing a > lockless check first etc. are all pretty standard and unless there is > something unusual going on (e.g. complicated circumstances leading to a > leak) there is not much to explain. In particular, I don't see why > anyone would explain why leaks are bad on each commit plugging one. Right; these (unused variable, resource leaks) usually do not warrant elaborate explanation. [ Some linefeeds below were trimmed for brevity ] > The gist is as follows: there are plenty of cases where the kernel wants > to atomically replace the value of a particular variable. Sometimes, > like in this commit, we want to bump the counter by 1, but only if the > current value is not 0. For that we need to read the value, see if it is > 0 and if not, try to replace what we read with what we read + 1. We > cannot just increment as the value could have changed to 0 in the > meantime. > But this also means that multiple cpus doing the same operation on the > same variable will trip on each other - one will succeed while the rest > will have to retry. > Prior to this commit, each retry attempt would explicitly re-read the > value. This induces cache coherency traffic slowing everyone down. > amd64 has the nice property of giving us the value it found eleminating > the need to explicitly re-read it. There is similar story on i386 and > sparc. > Other architectures may also benefit from this, but that I did not > benchmark. > > In short[,] under contention atomic_fcmpset is going to be faster than > atomic_cmpset. > I did not benchmark this particular change, but a switch of the sort > easily gives 10%+ in microbenchmarks on amd64. > That said, while one can argue this optimizes the code, it really > depessimizes it as something of the sort should have been already > employed. Given the above, IMHO it's quite far from an obvious or of manpage-lookup thing, and thus requires proper explanation in the commit log. > > While on this subject are there any official guidelines to writing > > commit messages, if no should we create some? > > I'm unaware of any. We might not have official guidelines, but 30%-what/70%-why rule would apply perfectly here. ;-) ./danfe