Date: Sun, 5 Feb 2017 15:17:46 +0000 From: Alexey Dokuchaev <danfe@FreeBSD.org> To: Mateusz Guzik <mjguzik@gmail.com> 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: <20170205151746.GA6841@FreeBSD.org> In-Reply-To: <20170205030006.GB4375@dft-labs.eu> References: <201702050140.v151eRXX090326@repo.freebsd.org> <42c790bb-df12-2a50-6181-24bac5c72d34@multiplay.co.uk> <20170205030006.GB4375@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170205151746.GA6841>