svn commit: r313260 - head/sys/kern

[Mateusz Guzik]

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>