Re: [PATCH 06/19] x86, mm: setup page table in top-down

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Fri, 19 Oct 2012, Yinghai Lu wrote:
> On Fri, Oct 19, 2012 at 9:24 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 18 Oct 2012, Yinghai Lu wrote:
> >> Get pgt_buf early from BRK, and use it to map PMD_SIZE from top at first.
> >> then use mapped pages to map more range below, and keep looping until
> >> all pages get mapped.
> >>
> >> alloc_low_page will use page from BRK at first, after that buff is used up,
> >> will use memblock to find and reserve page for page table usage.
> >>
> >> At last we could get rid of calculation and find early pgt related code.
> >>
> >> -v2: update to after fix_xen change,
> >>      also use MACRO for initial pgt_buf size and add comments with it.
> >> -v3: skip big reserved range in memblock.reserved near end.
> >> -v4: don't need fix_xen change now.
> >>
> >> Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >
> > The series is starting to get in good shape!
> > I tested it on a 2G and an 8G VM and it seems to be working fine.
> 
> domU on 32bit and 64bit?

domU 64bit


> > The most important thing to do now is testing it on different machines
> > (with and without xen) and writing better commit messages.
> > We can help you with the testing but you really need to write better
> > docs.
> 
> I tested on native one on 32bit, and 64bit.
> 
> Also xen dom0 32bit and 64bit.

Good, thanks!


> Changelog is always problem now. Looks like everyone is complaining about that.
> 
> Actually I already tried my best on that, really don't know what to do next.
> 
> >
> > In particular you should state in clear letters that alloc_low_page is
> > always going to return a page that is already mapped. You should write
> > it both in the commit message and in the code as a comment.
> > This is particularly important because it is going to become part of the
> > interface between the common x86 code and the other x86 subsystems like
> > Xen.
> 
> alloc_low_page() is used in arch/x86/mm/init*.c. How come it becomes
> interface to
> other subsystem?

I chose the wrong words.

I meant that always allocating pages from areas that are already mapped,
will become an assumption for other x86 subsystems like Xen.
One shouldn't just go ahead and change this assumption without changing
the subsystems too.


> I'm not sure if we really need that comment in code for that:
> ---
> the page that alloc_low_page return is directed mapped already, could
> use virtual address
> to access it.
> ---

I just want to make sure that 3 years from now, when somebody comes up
with a new great idea to improve the intial pagetable allocation, he
doesn't forget that changing alloc_low_page might break other subsystems.

So I think that a comment is required here and should explicitly
mention why it is important that alloc_low_page returns a mapped page.


> > Also, it wouldn't hurt to explain the overall design at the beginning of
> > the series: I shouldn't have to read your code to understand what you
> > are doing. I should read the description of the patch, understand what
> > you are doing, then go and check if the code actually does what you say
> > it does.
> 
> Actually I really don't like to read too long change log in commit.
> Changelog should be concise and precise.
> code change is more straightforward to me.

Many people don't think like that.
Of course you shouldn't document line by line changes in the commit
message but you should include a full explanation of your changes, like
I wrote before.