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
