Messages in this thread | | | Date | Sat, 26 Feb 2011 11:36:16 +0100 | From | Tejun Heo <> | Subject | Re: [PATCH 3/3] x86,mm,64bit: Round up memory boundary for init_memory_mapping_high() |
| |
On Fri, Feb 25, 2011 at 07:08:40PM -0800, Yinghai Lu wrote: > + end_pfn = round_up(end_pfn, data->align>>PAGE_SHIFT);
And now you're mapping beyond max_pfn without even noting the behavior change _anywhere_. What the hell? It's not like this point hasn't been brought up before. It has been mentioned _twice_ in this very thread. Come on.
<rant> From the previous responses, I suppose you wouldn't care for this advice but well I'll give it anyway. Spend time and effort documenting the changes and their rationales you make in the comments and changelog. Putting those things in words will force _yourself_ to think about whether the changes are accurate and justified in addition to helping other people understand and review the changes. And down the road, after several years, when someone, even yourself, needs to change the related code again, [s]he will be able to find out and understand how and why things are implemented much more easily.
In the second patch, you added @tbl_end and your explanation was @tbl_end could be shorter than @end. What is that? How is anyone supposed to understand what that means? You needed that change because the code currently depends on memory range to do NUMA affine allocation and when nodes are interleaved the rounding up may end up allocating page table from a different node. If you have put that in words, you would probably have recognized how lame and cryptic that piece of code is and other reviewers would also have much easier time understanding what that is doing and say no. And maybe this is too much to ask but why not add a nice docbook comment while you're adding an extra parameter?
At this point, I really find it difficult to take your patches seriously. They're cryptic, badly documented, and making behavior changes left and right and even when advised you don't even try to describe the changes and rationales. </rant>
I know you know the code and hardware and have keen eyes for details. _Please_ give it a bit more effort.
For this one, I think I'll just redo the patches and rip out the memblock iteration code. The complexity doesn't really seem justified.
Thank you.
-- tejun
| |