Re: [RFC] mm: Enable generic pfn_valid() to handle early sections with memmap holes

[Mike Rapoport]

On Thu, Mar 11, 2021 at 01:22:53PM +0530, Anshuman Khandual wrote:
> 
> On 3/8/21 2:25 PM, Mike Rapoport wrote:
> > Hi Anshuman,
> > 
> > On Mon, Mar 08, 2021 at 08:57:53AM +0530, Anshuman Khandual wrote:
> >> Platforms like arm and arm64 have redefined pfn_valid() because their early
> >> memory sections might have contained memmap holes caused by memblock areas
> >> tagged with MEMBLOCK_NOMAP, which should be skipped while validating a pfn
> >> for struct page backing. This scenario could be captured with a new option
> >> CONFIG_HAVE_EARLY_SECTION_MEMMAP_HOLES and then generic pfn_valid() can be
> >> improved to accommodate such platforms. This reduces overall code footprint
> >> and also improves maintainability.
> > 
> > I wonder whether arm64 would still need to free parts of its memmap after
> 
> free_unused_memmap() is applicable when CONFIG_SPARSEMEM_VMEMMAP is not enabled.
> I am not sure whether there still might be some platforms or boards which would
> benefit from this. Hence lets just keep this unchanged for now.

For builds with VMEMMAP off free_unused_memmap() would still provide some
memory savings. But the question is whether these savings are really
important for somebody? When the section size was 512M no doubt smaller
systems would waste a lot of memory for empty memory map. But with the
section size of 128M and the general increase in memory sizes even on
smaller devices we might be not actually saving anything. OTOH, we need to
keep arm64::pfn_valid() up to date with memory hot(un)plug, ZONE_DEVICE
etc.

Maybe as a compromise we can start with making free_unused_memmap() and
arm64::pfn_valid() only available in "legacy" mode that does not support
memory hotplug, pmem and so on?
 
> > the section size was reduced. Maybe the pain of arm64::pfn_valid() is not
> > worth the memory savings anymore?
> 
> arm64 pfn_valid() special case was primarily because of MEMBLOCK_NOMAP tagged
> memory areas, which are reserved by the firmware.

I'm confused now. AFAIU, pfn_valid() means that there is a struct page for that
pfn and nothing else. Why is that related to MEMBLOCK_NOMAP?

> > 
> >> Commit 4f5b0c178996 ("arm, arm64: move free_unused_memmap() to generic mm")
> >> had used CONFIG_HAVE_ARCH_PFN_VALID to gate free_unused_memmap(), which in
> >> turn had expanded its scope to new platforms like arc and m68k. Rather lets
> >> restrict back the scope for free_unused_memmap() to arm and arm64 platforms
> >> using this new config option i.e CONFIG_HAVE_EARLY_SECTION_MEMMAP.
> > 
> > The whole point of 4f5b0c178996 was to let arc and m68k to free unused
> > memory map with FLATMEM so they won't need DISCONTIGMEM or SPARSEMEM. So
> > whatever implementation there will be for arm/arm64, please keep arc and
> > m68k functionally intact.
> 
> Okay. Will protect free_unused_memmap() on HAVE_EARLY_SECTION_MEMMAP_HOLES
> config as well.
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index d9fa2e62ab7a..11b624e94127 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1927,8 +1927,11 @@ static void __init free_unused_memmap(void)
>         unsigned long start, end, prev_end = 0;
>         int i;
>  
> -       if (!IS_ENABLED(CONFIG_HAVE_EARLY_SECTION_MEMMAP_HOLES) ||
> -           IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP))
> +       if (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP))
> +               return;
> +
> +       if (!IS_ENABLED(CONFIG_HAVE_EARLY_SECTION_MEMMAP_HOLES) &&
> +           !IS_ENABLED(CONFIG_HAVE_ARCH_PFN_VALID))
>                 return;

-- 
Sincerely yours,
Mike.