The "patch review" session at the 2011 Kernel Summit went off in some
interesting directions, but it all started with Rafael Wysocki making the
claim that we simply
do not do enough patch review. How, he said, can we fix that? And how can
we get thorough review of patches? Just getting a Reviewed-by tag, he
said, is often not really enough to give confidence in a patch. That led
to some discussion of the relative meaning and merit of Reviewed-by and
Signed-off-by. The latter is a much stronger statement in the end, and, it
was agreed, carries more weight.
Ted Ts'o talked about the plight of developers who get a lot of
detail-oriented review which they duly address. Only later does a
subsystem maintainer come by and say that the basic premise of the patch is
wrong and that the previous revisions were for nothing. It would be
better, he said, if maintainers could perform high-level review of patches
quickly to help developers avoid useless work. Ingo Molnar disagreed,
saying that reviewer bandwidth is, in general, the limiting factor in
kernel development. So he insists that the code be clean and free of
stylistic problems before he will review it at all.
Another issue raised by Ted is ensuring that problems raised in previous
reviews have been addressed in a new revision of a patch set. Tools like
Gerrit, he said, can be most helpful in this regard. With Gerrit, it is
possible to look at the changes in a patch over time and to track the
comments that were made. At his work, use of Gerrit is mandated by
policy, and it helps a lot.
Johannes Berg said that there is more review happening than people think,
it's just that a lot of it is invisible. He looks at almost every wireless
patch that goes by but, if he sees nothing to complain about, he'll usually
just move on quietly. Responding to every patch, even if he has no
objections, would increase the amount of email flying around and slow
Peter Zijlstra complained that he has a whole mailbox full of patches that
he needs to review. Alan Cox corrected him: what Peter really has is a
mailbox of patches whose developers need to find reviewers for.
Trying to do all the review himself is never going to scale; that is a
lesson that has been learned many times in the kernel's history. It is
necessary to find a way to spread out the work.
Andrew Morton said that the quality of the reviews and Acked-by responses
is not always that great. Sometimes a patch will go out, only to be
followed by several Acked-by responses within a few minutes; he knows that
all of those people are not reviewing that patch in that time. Some of
those responses, he said, are clearly bogus. Peter suggested that
"corporate interest" can be behind that kind of coordinated response. But
Dave Airlie pointed out that quick acks can also happen if the patch has
already been through internal review and the developers involved already
know that they are happy with it. James Bottomley said that he'll often
ignore reviews that come from the same company as the original patch; he
will also ignore a Reviewed-by response that did not contain any actual
Alan said that part of the problem comes from development that has been
contracted out to other companies - often located in China or India - where
the contract requires that the resulting code be merged upstream. Ted
noted that this is actually an improvement - upstreaming code was once seen
as unimportant. But, he said, it would be good to get any in-house review
done in public rather than behind closed doors; he acknowledged that this
idea can be a hard sell in some companies. Alan observed that many of
those patches, in their early versions, are not something that most people
would want to see.
Rafael concluded that if there is a review problem, it is specific to some
subsystems. The maintainers of those subsystems will need to figure things
out. But, Ted said, a related problem is large patch sets that touch many
subsystems. Those patch sets may never get in at all, or they may see
review only for some parts of the set.
Andrew complained about the acceptance of entirely new features into the
kernel. Those features often land on his doorstep without much
justification, forcing him to ask the developers to explain their
motivations. The kernel community, he complained, is not supporting him
well. Who can tell him if a given patch makes sense? Mistakes have been
made in the past; bad features have been merged and good stuff has been
lost. How, he asked, can he find people who know better about the
desirability of specific patches?
Grant Likely suggested the possible creation of an architectural review
group that could consider patches as a whole. Andrew said that might help,
but it would have to meet frequently enough to respond to patches quickly.
Nobody took responsibility for organizing this group, so it would not be
surprising if the idea goes no farther.
When "no" becomes "yes"
Alan said that, once upon a time, Linus used to simply say "no" to a lot of
patches. If the distributors shipped it anyway for long enough, he would
usually give in and merge it anyway. It is not an infallible method, but
it provided a good indication of whether a patch is worthwhile or not.
Chris Mason observed that the distributors have changed; they no longer
want to ship out-of-tree code in their kernels, so this method has lost
some of its efficacy.
Inevitably somebody pointed out an important example of just this process
in action, though. Many of the changes in the Android kernel have been
blocked from merging into the mainline, but Android (which certainly
qualifies as a sort of distribution) shipped it anyway, and it now has
many millions of users. What does that say about how we should treat the
As one might imagine, the discussion became rather unfocused and fragmented
for a while. It came back together when Linus took the microphone and
stated that, simply, code that actually is used is the code that is
actually worth something.
The Android code is certainly being used; the in-kernel code aimed
at the same problems is just a vague idea that is worthless in comparison.
We should, he said, consider merging suspend blockers as a real option.
Even if it truly is crap, we've had crap in the kernel before. The code
does not get any better out of tree. Alan Cox agreed that it is probably a
good idea to merge that code. The interface is important and has a lot of
users; getting the code merged is the best way to fix the implementation.
Ingo also agreed, saying that when code has millions of users, we have to
say "yes" to it.
Tim Bird said that there is a project underway in the Consumer Electronics
Linux Forum to fix up the Android patches and resubmit them for merging.
That news came as a surprise to the others in the room; it seems that there
is an independent effort in Linaro to do the same thing. Hopefully one
result of this discussion is that these people will find each other and
Ted said that, as kernel developers, we often have an inflated idea of our
ability to block things by saying "no." We can't stop things that way;
they will happen anyway; Jesse Barnes added that we don't have the power to
stop product development. Ben Herrenschmidt (and others) noted that, for a
lot of hardware vendors, the requirement now is to provide an Android
kernel for their products - not a mainline kernel. There is a Linux fork
out there, and it is threatening to take over. We are, Ben said, shooting
ourselves in the foot by keeping that code out.
Linus said that it comes down to a marketing issue. Once upon a time, the
traditional Linux distributors used to ship a lot of out-of-tree code in
their quest to have the best kernel. They have gotten past that phase now
and can pat themselves on the back, but they did the same things that
Android is doing now. We should, he said, calm down about the whole thing.
There were no hard conclusions from the session. Given the way things
went, though, it would be seriously surprising not to see the Android code
seriously considered for merging in the 3.3 or 3.4 time frame. Android has
shown staying power, and much of the opposition appears to have moderated
over time. That is a surprising conclusion from a session on patch review,
but a worthwhile one nonetheless.
Next: Development process issues
to post comments)