Re: [PATCH v6 1/2] mm/memory_hotplug: optimize zone contiguous check when changing pfn range

[David Hildenbrand (Arm)]

On 8/10/26 16:04, David Hildenbrand (Arm) wrote:
>>
>> Hi David
>>
>> My understanding is that init_unavailable_range() initializes all
>> PFNs that satisfy pfn_valid(), but not all of them satisfy
>> pfn_to_online_page(), since some PFNs belong to subsections that are
>> not online.
> 
> Thanks for reminding me. I think the right direction is to finally clean up the
> pfn_valid() handling.
> 
>>
>> You previously mentioned:
>>
>>   pfn_valid() says early sections always have a full memmap, so even invalid
>>   subsections have a memmap. pfn_to_online_page() says an invalid subsection
>>   cannot be online and its content must be stale. for_each_valid_pfn() follows
>>   pfn_valid() semantics, and we use it to initialize memmap that is not going
>>   to be online and account it as pages_with_online_memmap, which is wrong.
>>
>>   The cleanest approach is to avoid allocating memmap for subsections, which
>>   also removes the special early-section handling from pfn_valid() and
>>   for_each_valid_pfn().
>>
>> I also share the concerns raised by Sashiko in the analysis below [1]:
>>
>>   Scanners like isolate_migratepages_block() will then blindly iterate through
>>   the pageblock and access the completely uninitialized struct pages of the hole,
>>   leading to functional errors or kernel panics when reading these zero-filled
>>   structures via macros like PageHuge() or page_zone().
>>
>> That's why we went with the current approach in v6 instead of your
>> earlier suggestion. I'd really appreciate your guidance on which
>> direction you think would be more appropriate.
> Let me take a stab at just having pfn_valid() / for_each_valid_pfn() respecting
> the subsection map.

... and that turns complicated very quickly. The problem is that we have some users,
in particular the buddy, that just assumes that MAX_PAGE_ORDER regions are fully
accessible.

The fun begins once we have MAX_PAGE_ORDER span multiple subsections. So we'd actually
want to initialize the memmap.

The pfn_valid() vs. pfn_to_online_page() inconsistency is really nasty :(

I mean, in init_unavailable_range() we could actually figure out fairly easily
whether we are dealing with holes where pfn_to_online_page() would succeed.

diff --git a/mm/mm_init.c b/mm/mm_init.c
index e9c4204b73adb..54e71e17f2c3c 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -843,11 +843,13 @@ static void __init init_unavailable_range(unsigned long spfn,
                                          int zone, int node)
 {
        unsigned long pfn;
-       u64 pgcnt = 0;
+       u64 pgcnt = 0, online_pgcnt = 0;
 
        for_each_valid_pfn(pfn, spfn, epfn) {
                __init_single_page(pfn_to_page(pfn), pfn, zone, node);
                __SetPageReserved(pfn_to_page(pfn));
+               if (pfn_to_online_page(pfn))
+                       online_pgcnt++;
                pgcnt++;
        }

If it's a problem performance-wise, we can always try optimizing by skipping
checks within the same (sub)section.


-- 
Cheers,

David