Re: [PATCH 3/3] x86,mm,64bit: Round up memory boundary for init_memory_mapping_high()

[Tejun Heo <tj@kernel.org>]

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