Wednesday, September 05, 2007

Andrew Morton on Kernel Quality

Andrew spent a little time clarifying the existing flow - primarily addressing defects first via email, but then capturing defects wihch are not quickly resolved in bugzilla. Andrew pointed out that many bugs get "lost", some people don't respond to Andrew's email to put their bug into bugzilla. If the bugs are sent to some smaller mailing list, the bug may also get lost. Also, bugs that aren't resolved within a couple of days are typically lost, and there is no way to find and track that defect afterwards. Andrew went through about 3500 lkml messages and found ~50 real-looking bug reports which were not responded to adequately or at all. And, Andrew spends a lot of time nagging people and is getting fairly fed up with it.

Linus believes that most people do not read the kernel mailing list any more. Which could be a problem that makes finding such defects hard to find. The proposal is to consider creating a list just for reporting kernel bugs.

Andrews' wrap up questions were: Should he nag more? Do we need a professional nagger? Why are people ignoring so many bug reports? Somone needs to monitor other mailing lists for unloved bug reports.

Code review is good - an hour of review of someone else's 100 person hour patch can help get that code accepted, increase its overall quality, and improve the quality of the submitter's future patches. But we are not doing enough - many patches in Andrew's inbox get no review or just trivial comments. Also, when Andrew commits a patch, he'll cc a few potential reviewers, which doesn't work too well. And, this policy is only used for patches going through the -mm tree when headed for mainline. Lots of stuff goes from developer, into the subsystem tree, then into mainline. The problem stems from git trees being pulled directly without sufficient review of the patches internally. The suggestion is to send patches for review when you merge them, not when you send them to Linus. That way it is fresher in people's minds and you don't have to do patch rework during the merge window.

Developers generally don't have incentive to review other's work. For instance, they have other stuff to work in, reviewing is hard, a bit dull, and rather thankless, and for many developers, it is not part of their job description.

Andrew pointed out that last year we agreed that subsystem maintainers would send out a summary of what they were putting into the next merge window during each rc period, but no one has actually been doing that. That would be useful.

Andrew proposed a strawman which would require a Reviewed-by: for merge. The merger decies whether it was adequate based on the quality of that review, availability of review notes (a link to email archive in changelog would be great), the identify of the reviewer. The reviewers reputation will accumulate credibility over time. Andrew's goal is to produce an exchange economy in which coders have incentive to review others' work. The consensus is to start using this process gradually and gently to see how it works, potentially making it mandatory if it works out well. Reviewed-by: does not mean "reviewed for whitespace" - but means "reviewed for correctness and content." Linus believes that there are many small patches and this process may be a bit cumbersome for small patches. In that case, Christoph suggests, the subsystem maintainer might be the default reviewer. There is still some debate that this will not work with small patches. There is still a proposal to try this and see how it works.

Labels:

0 Comments:

Post a Comment

Links to this post:

Create a Link

<< Home