Memory-management patch review
Michal Hocko started the session off by saying that too many patches get into Andrew Morton's ‑mm tree without proper review. Fully half of them, he said, lack an Acked-by or Reviewed-by tag. But that is only part of the problem: even when patches do carry tags indicating that review has been done, that review is often superficial at best, focusing on little details. Reviewers are not taking the time to think about the real problem, he said. As a result, MM developers are "building hacks on other hacks because nobody remembers why they were added in the first place".
As an example, he raised memory hotplug, and the care that is taken when shifting pages between memory zones. But much of that could be avoided by simply not assigning pages to zones as early as happens now. MM developers were used to working around this issue, he said, and so never really looked into it. In the end, this is turning the MM subsystem into an unmaintainable mess that is getting worse over time. How, he asked, can we get more review for MM patches, as befits a core kernel subsystem? How can we get review that really matters, and how can we force submitters to fix the problems that are found?
One option, Hocko said, is to make it mandatory that every MM patch have at least one review tag. That, he said, is likely to slow things down considerably. There are 100-150 MM patches merged in each development cycle; if the 50% or so of them without review tags are held back, a lot less will get merged. Is the community OK with that?
Kirill Shutemov said that, if reviews are required to get patches merged,
there will also need to be a way to get developers to do those reviews.
Hocko agreed, saying that few developers are reviewing patches now. Mel
![[Michal Hocko]](https://images.weserv.nl/?url=https%3A%2F%2Fstatic.lwn.net%2Fimages%2Fconf%2F2017%2Flsfmm%2FMichalHocko2-sm.jpg&q=12&output=webp&max-age=110 "Michal Hocko")
Gorman said that requiring reviews might be fair, but there should be one
exception: when developers modify their own code. In general, the
principal author should not need as much review for subsequent
modifications.
Morton said that a lot of patches do not really require review; many of them are trivial in nature. When review does happen, he said, the quality can vary considerably; there are some Reviewed-by tags that he doesn't believe at all. Gorman agreed that reviews need to have some merit to be counted.
Johannes Weiner worried that requiring reviews could cause work to fall through the cracks. Obscure bug fixes might not get merged, and memory-hotplug work could languish. Memory hotplug is a particular "problem child", Morton said; there is a lot of drive-by work and he has no idea who can review it. Quite a few people, Hocko added, are pursuing their own use case and don't really care about the rest. Part of the problem, Morton said, is that nobody really wants to clean up memory hotplug and, even if they did, they don't have the hardware platforms that would allow them to test the result.
Gorman said that it is important to start enforcing some sort of rule around review. Patches that still need review should have a special tag in the -mm tree. If the percentage of patches so tagged is too high when the -rc5 prepatch comes out, developers who have pending patches should be conscripted to do some review work. That would, at least, encourage the active developers to do a bit more review work.
Hocko then went back to the issue of trivial patches which, he said, are a bigger problem than many people think. Many of them are broken in obscure ways and create problems. Gorman suggested simply dropping trivial patches that have no user impact. Morton said that he could make an effort to be more careful when taking those patches, but that his attempts to get reviews for these patches are often ignored. If the people who have touched a certain file ignore a patch to it, Gorman said, that patch should just be dropped.
Morton replied that he is reluctant to mandate a system where it's impossible to get changes into the kernel if you can't get them reviewed. People get busy or take vacations, and many of those patches are changes that we want anyway. Dropping them would be detrimental to the kernel as a whole. Hocko said that XFS is now mandating reviews for all changes, and it doesn't appear to be suffering from patches being dropped on the floor.
The discussion then shifted to high-level design review, with Hocko saying that high-level review is hard and he wishes we had more of it, but it is not the biggest problem. The real issue is that we have more submitters of changes than reviewers of those changes. Morton said that he would push harder to get patches reviewed, and would do a walk-through around -rc5 to try to encourage review for specific patches needing it.
Morton said there are particular problems around specific patch sets that never seem to get enough review. Heterogeneous memory management is one of those; it is massive, and somebody spent a lot of time on it, but there don't seem to be a whole lot of other people who care about it. The longstanding ZONE_CMA patches are another example. There is a demand for this work, but it has been blocked, he said, partly because Gorman doesn't like it. Gorman replied that he still thinks it's not a good idea, and "you're going to get a kicking from it", but if the people who want that feature want to maintain it, they should go for it; it doesn't affect others. So he will not block the merging of that code.
Hocko raised the topic of the hugetlbfs code, which is complex to the point that few developers want to touch it. Perhaps, he said, hugetlbfs should be put into maintenance mode with no new features allowed. The consensus on this idea seemed to be that the MM developers should say "no more" to changes in this area, but not try to impose strict rules.
Another conclusion came from Morton, who encouraged the MM developers to be more vocal on the mailing lists. The volume on the linux-mm list is reasonable, so there is no real excuse for not paying attention to what is happening there. Developers should, he said, "hit reply more often". Gorman agreed, but said that there need to be consequences from those replies; if a developer pushes back on a patch, that patch needs to be rethought.
By that time, the end of LSFMM was in sight, and thoughts of beer began to
take over. Whether this discussion leads to better review of MM patches
remains to be seen, but it has, if nothing else, increased awareness of the
problem.
| Index entries for this article | |
|---|---|
| Kernel | Development model/Code review |
| Conference | Storage, Filesystem, and Memory-Management Summit/2017 |
Posted Mar 30, 2017 6:38 UTC (Thu)
by blackwood (guest, #44174)
[Link] (4 responses)
I very strongly disagree with this statement, from our experience in the drm subsystem, for bunch of reasons:
Because of this we have a hard rule in drm/i915 and the drm core that you don't get to merge your own crap without review, ever. We're rolling that rule out (slowly) to all the drivers now, which means we're kickstarting a much bigger review market for drm drivers. It works (just takes a bit of time).
Posted Mar 30, 2017 9:27 UTC (Thu)
by farnz (subscriber, #17727)
[Link] (3 responses)
I've seen in code review elsewhere that it's often good to have the new person review the expert's code. Two interesting things happen:
Posted Apr 12, 2017 20:44 UTC (Wed)
by fest3er (guest, #60379)
[Link] (2 responses)
Remember that interpersonal written/verbal communication *is* programming; the speaker/writer is programming the listener/reader to construct a mental image. Thus, the human brain, being the pattern matcher that it is, will construct a 'picture'. If the brain has received adequate instructions , the picture will be largely complete and apparent flaws will stand out.
Code must not only be reviewed. It must also be reviewable.
Posted Apr 12, 2017 23:44 UTC (Wed)
by nix (subscriber, #2304)
[Link] (1 responses)
Posted Apr 15, 2017 9:23 UTC (Sat)
by sdalley (subscriber, #18550)
[Link]
You might be terrible at "interpersonal communication", (by which I assume you mean spoken communication with aural/visual cues) but from what I've seen, your written communication is superb. It also gives, may I say, a clear (and edifying) impression of the person behind it.
Good documentation (and authoring in general) requires being able to think yourself into the shoes of the reader. Some are better at that than others, and practice makes perfect, but it's always going to require conscious thought and effort. Good engineering communication, rather than just code-churning, is no exception.
Memory-management patch review
- If your intimitadely familiar with some code you tend to have an illusion that you know everything. I stopped counting the embarrassing stuff I've missed in patches for code I entirely designed myself.
- If your the expert, you need to share your knowledge. Best way to do that is by explaining all the tricky details to a reviewer, and the reviewer making sure it's all properly documented, commented as needed and functions/variables all have meaningful names fully capturing all the details.
- Often author == maintainer, so your primary reviewer. If you want to kickstart a real, working review market in your community, then the easiest way to pull that off is by subjecting yourself to it. That way new folks learn the art of review, and there's a direct benefit for them since you can return the favour in reviewing their patches. After a few months/years and increasingly encouraging contributors to just cross-review you have a review market where you as maintainer can entirely concentrate on high-level design issues.
- On top, if review is only a hazing ritual for outsiders and doesn't apply for core contributors, it will discourage new contributors. Leading by example and all that.
Memory-management patch review
Memory-management patch review
Memory-management patch review
Remember that interpersonal written/verbal communication *is* programming; the speaker/writer is programming the listener/reader to construct a mental image. Thus, the human brain, being the pattern matcher that it is, will construct a 'picture'. If the brain has received adequate instructions, the picture will be largely complete and apparent flaws will stand out.
For most people this metaphor is just completely wrong. Programming is conscious and intentional: interpersonal communication is a theory-of-mind thing for which there is inbuilt neural assistance, and determining how to modify that model is not done consciously. (Which is a good thing, because those of us who do have to do it consciously are mostly terrible at it as a result.)
Memory-management patch review