Re: [PATCH 2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

[David Hildenbrand <david@redhat.com>]

On 12.07.22 06:23, Li Chen wrote:
> Hi David,
>  ---- On Mon, 11 Jul 2022 22:53:36 +0800  David Hildenbrand <david@redhat.com> wrote --- 
>  > On 11.07.22 14:24, Li Chen wrote:
>  > > From: Li Chen <lchen@ambarella.com>
>  > > 
>  > > When mhp use sparse_add_section, don't check no-map region,
>  > > so that to allow no-map reserved memory to get struct page
>  > > support.
>  > > 
>  > > Signed-off-by: Li Chen <lchen@ambarella.com>
>  > > Change-Id: I0d2673cec1b66adf695251037a00c240976b226f
>  > > ---
>  > >  mm/sparse.c | 4 +++-
>  > >  1 file changed, 3 insertions(+), 1 deletion(-)
>  > > 
>  > > diff --git a/mm/sparse.c b/mm/sparse.c
>  > > index 120bc8ea5293..a29cd1e7014f 100644
>  > > --- a/mm/sparse.c
>  > > +++ b/mm/sparse.c
>  > > @@ -690,7 +690,9 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
>  > >  
>  > >      if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
>  > >          rc = -EINVAL;
>  > > -    else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
>  > > +    else if (memblock_is_map_memory(PFN_PHYS(pfn)) &&
>  > > +         bitmap_intersects(map, subsection_map,
>  > > +                   SUBSECTIONS_PER_SECTION))
>  > >          rc = -EEXIST;
>  > >      else
>  > >          bitmap_or(subsection_map, map, subsection_map,
>  > 
>  > I'm not sure I follow completely what you are trying to achieve. But if
>  > you have to add memblock hacks into mm/sparse.c you're most probably
>  > doing something wrong.
>  > 
>  > Please explain why that change is necessary, and why it is safe.
> 
> In the current sparse memory model, free_area_init will insert all memblock.memory into subsection_map and no-map rmem is also a 
> memblock.memory. So, without this change, fill_subsection_map will return -EEXIST.
> 
> I would say it's not a good idea to insert no-map memblock into subsection_map, and I have no idea why sparse do this.
> So, I simply skip no-map region here.

The thing is:

if the subsection map is set, then there already *is* a memmap and you
would simply be ignoring it (and overwriting a memmap in e.g.,
ZONE_NORMAL to be in ZONE_DEVICE suddenly, which is wrong).


Reading memblock_mark_nomap():

"The memory regions marked with %MEMBLOCK_NOMAP will not be added to the
direct mapping of the physical memory. These regions will still be
covered by the memory map. The struct page representing NOMAP memory
frames in the memory map will be PageReserved()"


So having a memmap for these ranges is expected, and a direct map is not
desired. What you propose is a hack. You either have to reuse the
existing memmap (which is !ZONE_DEVICE -- not sure if that's a problem)
or we'd have to look into teaching init code to not allocate a memmap
for sub-sections that are fully nomap.

But not sure who depends on the existing memmap for nomap memory.

> 
> As for safety:
> 1. The caller of fill_subsection_map are mhp and *_memremap_pages functions, no-map regions are not related to them, so existing codes won't be broken.
> 2. This change doesn't change memblock and subsection_map.
> 

Sorry, but AFAIKT it's a hack and we need a clean way to deal with nomap
memory that already has a memmap instead.


-- 
Thanks,

David / dhildenb