Re: [PATCH v5 1/5] mm,memory_hotplug: Allocate memmap from the added memory range

[Oscar Salvador]

On Wed, Mar 24, 2021 at 08:16:53PM +0100, David Hildenbrand wrote:
> > > 1. If the underlying memory block is offline, all sections are offline. Zone
> > > shrinking code will happily skip over the vmemmap pages and you can end up
> > > with out-of-zone pages assigned to the zone. Can happen in corner cases.
> > 
> > You are right. But do we really care? Those pages should be of no
> > interest to anybody iterating through zones/nodes anyway.
> 
> Well, we were just discussing getting zone/node links + span right for all
> pages (including for special reserved pages), because it already resulted in
> BUGs. So I am not convinced that we *don't* have to care.
> 
> However, I agree that most code that cares about node/zone spans shouldn't
> care - e.g., never call set_pfnblock_flags_mask() on such blocks.
> 
> But I guess there are corner cases where we would end up with
> zone_is_empty() == true, not sure what that effect would be ... at least the
> node cannot vanish as we disallow offlining it while we have a memory block
> linked to it.

Having quickly looked at Michal's change, I have to say that it does not
look that bad, but I think it is doing the initialization/accounting at
the wrong stage, plus the fact that I dislike to place those pages in
ZONE_NORMAL, although they are not movable.
But I think the vmemmap pages should lay within the same zone the pages
they describe, doing so simplifies things, and I do not see any outright
downside.

Back in the old days, we used to have this sort of
unitilization/tearing-down of pages at hot-add/hot-remove stage, and we
moved from there because of many problems we had.
While we might not encounter those problems with vmemmap pages because of
their nature, I think that not sticking with the right place might bring
problems as David outlined.

IMHO, initizalization and accounting should be done in online/offline
stage.
Now, the problems seems to be that doing it right in
offline_pages()/online_pages() seems a bad practice and conflates with
different concept.

This is just an idea I did not get to think carefully, but what if we
do it in helpers right before calling online_pages()/offline_pages()
in memory_block_action() ?
I have to confess that I do not really like the idea, but would solve do
the right things at the right stage.

One last thing.
Let us try to avoid things like "we do not care". Hotplug code was full
of "we do not care" assumptions that bit us in the ass at some point.



-- 
Oscar Salvador
SUSE L3