Messages in this thread | | | Date | Fri, 19 Oct 2012 09:41:22 -0700 | Subject | Re: [PATCH 06/19] x86, mm: setup page table in top-down | From | Yinghai Lu <> |
| |
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?
> > > 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.
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'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. ---
> > 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.
Thanks
Yinghai
| |