Re: [PATCH v1 11/11] mm/memory_hotplug: mark pageblocks MIGRATE_ISOLATE while onlining memory
From: Michal Hocko <mhocko@suse.com>
Date: 2020-08-19 13:25:15
Also in:
lkml
On Wed 19-08-20 12:11:57, David Hildenbrand wrote:
Currently, it can happen that pages are allocated (and freed) via the buddy before we finished basic memory onlining. For example, pages are exposed to the buddy and can be allocated before we actually mark the sections online. Allocated pages could suddenly fail pfn_to_online_page() checks. We had similar issues with pcp handling, when pages are allocated+freed before we reach zone_pcp_update() in online_pages() [1]. Instead, mark all pageblocks MIGRATE_ISOLATE, such that allocations are impossible. Once done with the heavy lifting, use undo_isolate_page_range() to move the pages to the MIGRATE_MOVABLE freelist, marking them ready for allocation. Similar to offline_pages(), we have to manually adjust zone->nr_isolate_pageblock. [1] https://lkml.kernel.org/r/1597150703-19003-1-git-send-email-charante@codeaurora.org Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Michal Hocko <mhocko@suse.com> Cc: Wei Yang <redacted> Cc: Baoquan He <redacted> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> Cc: Oscar Salvador <osalvador@suse.de> Cc: Charan Teja Reddy <redacted> Signed-off-by: David Hildenbrand <redacted>
Acked-by: Michal Hocko <mhocko@suse.com> Yes this looks very sensible and we should have done that from the beginning. I just have one minor comment below
quoted hunk ↗ jump to hunk
@@ -816,6 +816,14 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, if (ret) goto failed_addition; + /* + * Fixup the number of isolated pageblocks before marking the sections + * onlining, such that undo_isolate_page_range() works correctly. + */ + spin_lock_irqsave(&zone->lock, flags); + zone->nr_isolate_pageblock += nr_pages / pageblock_nr_pages; + spin_unlock_irqrestore(&zone->lock, flags); +
I am not entirely happy about this. I am wondering whether it would make more sense to keep the counter in sync already in memmap_init_zone. Sure we add a branch to the boot time initialization - and it always fails there - but the code would be cleaner and we wouldn't have to do tricks like this in caller(s). -- Michal Hocko SUSE Labs