Date: Sun, 5 Feb 2017 04:00:06 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Steven Hartland <steven.hartland@multiplay.co.uk> Cc: Mateusz Guzik <mjg@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313260 - head/sys/kern Message-ID: <20170205030006.GB4375@dft-labs.eu> In-Reply-To: <42c790bb-df12-2a50-6181-24bac5c72d34@multiplay.co.uk> References: <201702050140.v151eRXX090326@repo.freebsd.org> <42c790bb-df12-2a50-6181-24bac5c72d34@multiplay.co.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Feb 05, 2017 at 02:16:12AM +0000, Steven Hartland wrote: > Hi Mateusz could you improve on the commit message as it currently > describes what is changed, which can be obtained from the diff, but > not why? > > I hope on one feels like I'm trying to teach them to suck eggs, as I > know everyone here has a wealth of experience, but I strongly believe > commit messages are a very important way of improving the overall > quality of the code base by sharing with others the reason for > changes, which they can then learn from. I know I for one love > picking up new nuggets of knowledge from others in this way. > In general I agree that commit messages should be descriptive (see below), but I also think there are things which are somewhat self-explanatory and if someone is unfamiliar with given concept, they can always ask and/or find it documented elsewhere. 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. In the same spirit, the switch to fcmpset should be a routine change. Interested parties can check the man page and the commit message which introduced it. Arguably I should have elaborated more in there. 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. Parties interested in scalability problems (and cpu caches in particular) are encouraged to read: https://www.kernel.org/pub/linux/kernel/people/paulmck/perfbook/perfbook.html > Also I believe this is area the project as a whole can improve on, so > I don't mean to single out anyone here. > > Anyway I hope people find this useful: > > When I write a commit message I try to stick to the following rules > which I believe helps to bring clarity for others about my actions. > 1. First line is a brief summary of the out come of the change e.g. > Fixed compiler warnings in nvmecontrol on 32bit platforms This should be mandatory in the form of one sentence, preferably prefixed with a keyword. So in particular I would phrase this one as: nvme: fix compiler warnings in nvmecontrol on 32bit platforms or so > 2. Follow up paragraphs expand on #1 if needed including details > about not just what but why the change was made e.g. > Use ssize_t instead of uint32_t to prevent warnings about a > comparison with different signs. Due to the promotion rules, this > would only happen on 32-bit platforms. See above. Unclear how much explanation is needed. While a safe default would be to always elaborate, I don't think it is necessary in all cases. Things which do need elaborated commit messages is algorithm changes, bug fixes (unless the bug is trivial), > 3. When writing #2 include details that would not be obvious to > non-experts in the particular area. > > #2 and #3 are really important to sharing knowledge that others may > not know, its quite relevant to this commit msg, as while it may be > obvious to you and others familiar with the atomic ops, to the rest > of us we're just wondering why make this change? > Well, while I don't have the best track record with commit messages, I do believe I elaborate enough when it is needed, which I think is rare. see https://svnweb.freebsd.org/base?view=revision&revision=306608 or https://svnweb.freebsd.org/base?view=revision&revision=303643 or https://svnweb.freebsd.org/base?view=revision&revision=295233 for examples Other than that I think my changes are straightforward (and sometimes weirdly buggy :/). tl;dr I think my commit message here was perfectly fine > N.B. The example is based on Warner's recent commit purely as an > example, which had a good why, just missing the brief summary. > > While on this subject are there any official guidelines to writing > commit messages, if no should we create some? > I'm unaware of any. > On 05/02/2017 01:40, Mateusz Guzik wrote: > >Author: mjg > >Date: Sun Feb 5 01:40:27 2017 > >New Revision: 313260 > >URL: https://svnweb.freebsd.org/changeset/base/313260 > > > >Log: > > fd: switch fget_unlocked to atomic_fcmpset > > > >Modified: > > head/sys/kern/kern_descrip.c > > > >Modified: head/sys/kern/kern_descrip.c > >============================================================================== > >--- head/sys/kern/kern_descrip.c Sun Feb 5 01:20:39 2017 (r313259) > >+++ head/sys/kern/kern_descrip.c Sun Feb 5 01:40:27 2017 (r313260) > >@@ -2569,8 +2569,8 @@ fget_unlocked(struct filedesc *fdp, int > > if (error != 0) > > return (error); > > #endif > >- retry: > > count = fp->f_count; > >+ retry: > > if (count == 0) { > > /* > > * Force a reload. Other thread could reallocate the > >@@ -2584,7 +2584,7 @@ fget_unlocked(struct filedesc *fdp, int > > * Use an acquire barrier to force re-reading of fdt so it is > > * refreshed for verification. > > */ > >- if (atomic_cmpset_acq_int(&fp->f_count, count, count + 1) == 0) > >+ if (atomic_fcmpset_acq_int(&fp->f_count, &count, count + 1) == 0) > > goto retry; > > fdt = fdp->fd_files; > > #ifdef CAPABILITIES > > > -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170205030006.GB4375>