Date: Tue, 7 Feb 2017 15:57:50 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Alexey Dokuchaev <danfe@FreeBSD.org> Cc: Steven Hartland <steven.hartland@multiplay.co.uk>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@FreeBSD.org> Subject: Re: svn commit: r313260 - head/sys/kern Message-ID: <20170207145750.GD4772@dft-labs.eu> In-Reply-To: <20170205151746.GA6841@FreeBSD.org> References: <201702050140.v151eRXX090326@repo.freebsd.org> <42c790bb-df12-2a50-6181-24bac5c72d34@multiplay.co.uk> <20170205030006.GB4375@dft-labs.eu> <20170205151746.GA6841@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Feb 05, 2017 at 03:17:46PM +0000, Alexey Dokuchaev wrote: > 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. > If the aformenteiond explanation is necessary, the place for it is in the man page. There are already several commits with fcmpset and there will be more to come. I don't see why any of them would convey the information. > > > 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 -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170207145750.GD4772>