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> _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"