lkml.org 
[lkml]   [2012]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v8 21/46] x86, mm: setup page table in top-down
On Wed, Nov 28, 2012 at 12:16:16PM -0800, Yinghai Lu wrote:
> On Wed, Nov 28, 2012 at 9:50 AM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> >> /*
> >> - * Iterate through E820 memory map and create direct mappings for only E820_RAM
> >> - * regions. We cannot simply create direct mappings for all pfns from
> >> - * [0 to max_low_pfn) and [4GB to max_pfn) because of possible memory holes in
> >> - * high addresses that cannot be marked as UC by fixed/variable range MTRRs.
> >> - * Depending on the alignment of E820 ranges, this may possibly result in using
> >> - * smaller size (i.e. 4K instead of 2M or 1G) page tables.
> >> + * would have hole in the middle or ends, and only ram parts will be mapped.
> >
> >
> > What? What is the 'would' refering to? Why remove a good comment that explains
> > the function. Why not just modify it a bit please?
> >
>
> ==> update to
>
> /*
> * We need to iterate through E820 memory map and create direct mappings
> * for only E820_RAM and E820_KERN_RESERVED regions. We cannot simply
> * create direct mappings for all pfns from [0 to max_low_pfn) and
> * [4GB to max_pfn) because of possible memory holes in high addresses
> * that cannot be marked as UC by fixed/variable range MTRRs.
> * Depending on the alignment of E820 ranges, this may possibly result
> * in using smaller size (i.e. 4K instead of 2M or 1G) page tables.
> *
> * init_mem_mapping call init_range_memory_mapping with big range.
> * That range would have hole in the middle or ends, and only ram parts
> * will be mapped in init_range_memory_mapping.
> */
>
>
>
> >> - max_pfn_mapped = 0; /* will get exact value next */
> >> /* the ISA range is always mapped regardless of memory holes */
> >> init_memory_mapping(0, ISA_END_ADDRESS);
> >> - init_range_memory_mapping(ISA_END_ADDRESS, end);
> >> +
> >> + /* xen has big range in reserved near end of ram, skip it at first */
> >
> > I am not seeing the logic for doing it? The loop is quite generic
> > in doing it in reverse order, and the memblock_find_in_range
> > gets a nice PMD_SIZE region from the end of the memory.
> >
> > If the memory at the end is reserved, then it looks like it won't
> > be even considered in the loop, but it does get included in the fallback:
> >
> > if (real_end < end)
> > init_range_memory_mapping(real_end, end);
>
> that reserved in in memblock.reserved and it is not in e820.
>
> so memblock.memory will have that range too. then if we use all of
> first 2M to map
>
> those reserved range, we would not have enough mapped pages to be used
> as new page tables.

You should include that nice explanation as part of the comment. It is
rather suddle (or would be for me in 6 months when I would look at this
code).

>
> >
> >
> >
> >> + addr = memblock_find_in_range(ISA_END_ADDRESS, end, PMD_SIZE,
> >> + PAGE_SIZE);
> >> + real_end = addr + PMD_SIZE;
> >> +
> >> + /* step_size need to be small so pgt_buf from BRK could cover it */
> >> + step_size = PMD_SIZE;
> >> + max_pfn_mapped = 0; /* will get exact value next */
> >> + min_pfn_mapped = real_end >> PAGE_SHIFT;
> >> + last_start = start = real_end;
> >
> > Everytime I look at this loop, I keep on forgetting that it goes in reverse.
> > I am not sure if it is just me, but it might be useful for other
> > folks who are going to look at this in a year or so to have
> > a little hint:
> >
> > N.B. We start from the top (end of memory) and go to the bottom. The
> > memblock_find_in_range gets us a block of RAM from the end
> > of RAM.
>
> put the that in the comments.


\
 
 \ /
  Last update: 2012-12-06 00:01    [W:0.087 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site