On Linux kernel maintainer scalability

Posted Oct 13, 2016 8:38 UTC (Thu) by vegard (subscriber, #52330)
Parent article: On Linux kernel maintainer scalability

Regarding reviews and patch quality, this is an old email I have from Andrew Morton on the topic:

"""
Date: Wed, 16 Jul 2008 17:10:52 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: "Vegard Nossum" <vegard.nossum@gmail.com>
Cc: penberg@cs.helsinki.fi, torvalds@linux-foundation.org, mingo@elte.hu
Subject: Re: [git pull] RCU updates for v2.6.27

On Thu, 17 Jul 2008 01:47:05 +0200
"Vegard Nossum" <vegard.nossum@gmail.com> wrote:

> How about starting to reject patches which have no reviewed-by tag?

Yeah, this is sorely tempting.

At kernel-summit-07 I discussed and basically proposed the Reviewed-by:
thing and everyone was surprisingly agreeable. I think I floated the
idea of making it obligatory but in no way have I pushed it at all. I
just don't have the energy and the grief-absorption-capability.

It would be a huuuuuge change. A dramatic one. It would quickly cause
the organisation of an economy in which people find they need to
"trade" reviewing activity. People would understand that if they don't
review other people's stuff then others won't review their stuff and
they don't get their patches merged.

Would it increase kernel quality? Yes, quite a bit I expect.

Would it slow things down? Yes, it would.

But both of those are the same thing.

Should we do it? Well, yes, I think we should. There would be some
bumps and glitches but once we got into the swing of it, the impact
would be not too bad. After all, it is just a formalisation of
something which we're supposed to be doing already! If we were doing
that which we _know_ we should be doing, the impact would be zero.

_can_ we do it? Well, the steps are:

1: gather general consensus that we have some problem

2: get agreement and that the problem needs fixing

3: get agreement that this process step is one way of fixing said problem

4: implement it.

I think that we'd have a good chance of succeeding in all these steps.
After all, the winning debating point here is that *we're already
supposed to be reviewing all patches*.

Problem is, do I have the requisite energy to try to inflict this on
everyone at KS08? It's looking doubtful at present. It's much easier
if Linus just stands up and says "you all suck and we're doing this".
"""

I really do think reviews are undervalued in the sense that the reviewer often don't get very much out of it for themselves. You can put hours into a review but that is not recognised beyond a single "Reviewed-by" line in the final commit; in terms of personal exposure (which I do believe is a big factor for non-maintainer contributors, for better or worse), it is much more attractive to write code and patches.

Maybe maintainers should include review and reviewer stats in their pull requests, crediting not just the patch authors, but also the reviewers. This would also make it more obvious when large batches of patches have not been reviewed.

to post comments

On Linux kernel maintainer scalability

Posted Oct 13, 2016 8:41 UTC (Thu) by vegard (subscriber, #52330) [Link] (1 responses)

> You can put hours into a review but that is not recognised beyond a single "Reviewed-by" line in the final commit; in terms of personal exposure (which I do believe is a big factor for non-maintainer contributors, for better or worse), it is much more attractive to write code and patches.

(Not to mention commits which have already been made. I occasionally look over a pull request when I see it on LKML, but by that time there is no way to record the review in git history.)

On Linux kernel maintainer scalability

Posted Oct 17, 2016 14:51 UTC (Mon) by imMute (guest, #96323) [Link]

Maybe code reviews should be marked using tags instead of commit message strings - that way they can be added after-the-fact.
I suspect Git's refspec will need to learn to "exclude" things rather than being inclusive only, otherwise people who pull tags will get overloaded with them.

On Linux kernel maintainer scalability

Posted Oct 13, 2016 9:58 UTC (Thu) by blackwood (guest, #44174) [Link]

I wasn't aware of this mail from Andrew, but this is what I've done in i915, and essentially what's going on in the drm subsystem at large. Reviews are required, period. And yes we have a flourishing tit-for-tat review market going on, coordinated mostly over irc ;-)

One key bit to make that happen imo is that the maintainer (even with committer model) is a great example, and goes to great pains to get a review even for trivial patches, if those patches are their own. Otherwise everyone stops bothering with haggling for some reviewer's bandwidth real fast.

On Linux kernel maintainer scalability

Posted Oct 18, 2016 9:02 UTC (Tue) by wsa (guest, #52415) [Link]

Thank you for pointing out Andrew's mail. I couldn't agree more. What I forgot to mention in my talks: I2C will require "Reviewed-by" tags in the near future as well. Let's see how that works. The idea of mentioning reviewers in pull-requests is great, I'll definately do that!