Re: [PATCH v1 1/2] mm/page_alloc.c: Don't set pages PageReserved() when offlining

[David Hildenbrand]

On 21.10.19 16:43, Michal Hocko wrote:
> On Mon 21-10-19 16:19:25, David Hildenbrand wrote:
> > We call __offline_isolated_pages() from __offline_pages() after all
> > pages were isolated and are either free (PageBuddy()) or PageHWPoison.
> > Nothing can stop us from offlining memory at this point.
> >
> > In __offline_isolated_pages() we first set all affected memory sections
> > offline (offline_mem_sections(pfn, end_pfn)), to mark the memmap as
> > invalid (pfn_to_online_page() will no longer succeed), and then walk over
> > all pages to pull the free pages from the free lists (to the isolated
> > free lists, to be precise).
> >
> > Note that re-onlining a memory block will result in the whole memmap
> > getting reinitialized, overwriting any old state. We already poision the
> > memmap when offlining is complete to find any access to
> > stale/uninitialized memmaps.
> >
> > So, setting the pages PageReserved() is not helpful. The memap is marked
> > offline and all pageblocks are isolated. As soon as offline, the memmap
> > is stale either way.
> >
> > This looks like a leftover from ancient times where we initialized the
> > memmap when adding memory and not when onlining it (the pages were set
> > PageReserved so re-onling would work as expected).
> >
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Michal Hocko <mhocko@xxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Oscar Salvador <osalvador@xxxxxxx>
> > Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> > Cc: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Cc: Wei Yang <richard.weiyang@xxxxxxxxx>
> > Cc: Alexander Duyck <alexander.h.duyck@xxxxxxxxxxxxxxx>
> > Cc: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> > Cc: Pavel Tatashin <pavel.tatashin@xxxxxxxxxxxxx>
> > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
>
> We still set PageReserved before onlining pages and that one should be
> good to go as well (memmap_init_zone).
> Thanks!

memmap_init_zone() is called when onlining memory. There, set all pages 
to reserved right now (on context == MEMMAP_HOTPLUG). We clear 
PG_reserved when onlining a page to the buddy (e.g., 
generic_online_page). If we would online a memory block with holes, we 
would want to keep all such pages (!pfn_valid()) set to reserved. Also, 
there might be other side effects.

So it might not be that easy to remove. A cleanup that I have on my list 
is to disallow offlining memory blocks with holes. This implies that we 
will never online memory blocks with holes. This allows for some 
cleanups in the onlining/offlining code. For example, it would allow to 
get rid of this PG_reserved initialization.

I don't think that we have to support offlining memory blocks with 
holes. This can only be bootmem (never hotplugged memory), where the 
chance for this to work is in my opinion already not too good.

What's your opinion on this?

> There is a comment above offline_isolated_pages_cb that should be
> removed as well.

Right, I'll convert that comment

"Mark all sections offline and remove all free pages from the buddy."

Thanks!


--

Thanks,

David / dhildenb