Re: [PATCH v8 21/46] x86, mm: setup page table in top-down

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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.