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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/21/26 05:17, Liu, Yuan1 wrote:
>> -----Original Message-----
>> From: David Hildenbrand (Arm) <david@kernel.org>
>> Sent: Friday, August 21, 2026 1:46 AM
>> To: Liu, Yuan1 <yuan1.liu@intel.com>; Oscar Salvador <osalvador@suse.de>;
>> Mike Rapoport <rppt@kernel.org>; Wei Yang <richard.weiyang@gmail.com>
>> Cc: linux-mm@kvack.org; Zou, Nanhai <nanhai.zou@intel.com>; Chen Zhang
>> <zhangchen.kidd@jd.com>; Zeng, Jason <jason.zeng@intel.com>; Chen, Yu C
>> <yu.c.chen@intel.com>; Deng, Pan <pan.deng@intel.com>; Li, Tianyou
>> <tianyou.li@intel.com>; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v7 2/2] mm/memory_hotplug: optimize zone contiguous
>> check when changing pfn range
>>
>>>
>>> I think there may be an issue here: PAGE_SUBSECTION_MASK is not
>>> defined when CONFIG_FLATMEM is enabled, which would result in a
>>> build failure.
>>
>> Ah, yeah.
>>
>>>
>> until
>> actually
>>>
>>> Hi David
>>>
>>> What about the following approach? It removes the helper and changes
>>> the per-PFN online check to a per-chunk check.
>>>
>>> +       u64 pgcnt = 0, online_pgcnt = 0;
>>> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
>>> +       const unsigned long chunk = PAGES_PER_SUBSECTION;
>>> +#else
>>> +       const unsigned long chunk = epfn - spfn;
>>> +#endif
>>>
>>> -       for_each_valid_pfn(pfn, spfn, epfn) {
>>> -               __init_single_page(pfn_to_page(pfn), pfn, zone, node);
>>> -               __SetPageReserved(pfn_to_page(pfn));
>>> -               pgcnt++;
>>> +       /*
>>> +        * With VMEMMAP, subsection-sized holes can exist, and PFNs
>> within
>>> +        * these holes can fail pfn_to_online_page(). Without VMEMMAP,
>> we
>>> +        * always only have early sections when they are actually
>> online.
>>> +        */
>>> +       for (pfn = spfn; pfn < epfn; pfn += chunk) {
>>> +               const unsigned long chunk_epfn = min(pfn + chunk, epfn);
>>> +               const bool is_online
>> = !IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) ||
>>> +                                      pfn_to_online_page(pfn);
>>> +               unsigned long p;
>>> +
>>> +               for_each_valid_pfn(p, pfn, chunk_epfn) {
>>> +                       __init_single_page(pfn_to_page(p), p, zone,
>> node);
>>> +                       __SetPageReserved(pfn_to_page(p));
>>> +                       if (is_online)
>>> +                               online_pgcnt++;
>>> +                       pgcnt++;
>>> +               }
>>
>> I'd keep the for_each_valid_pfn() the outer loop.
> 
> Hi David
> 
> Do you mean the following approach? We use for_each_valid_pfn as 
> the outer loop and, with VMEMMAP,check online PFNs 
> in PAGES_PER_SUBSECTION chunks
> 
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
> +       const unsigned long chunk = PAGES_PER_SUBSECTION;
> +#else
> +       const unsigned long chunk = epfn - spfn;
> +#endif
> +       unsigned long pfn, next_chunk_pfn = spfn;
> +       u64 pgcnt = 0, online_pgcnt = 0;
> +       bool is_online = true;
> 
>         for_each_valid_pfn(pfn, spfn, epfn) {
>                 __init_single_page(pfn_to_page(pfn), pfn, zone, node);
>                 __SetPageReserved(pfn_to_page(pfn));
> +
> +               /*
> +                * With VMEMMAP, subsection-sized holes can exist, and PFNs
> +                * within these holes can fail pfn_to_online_page(). Without
> +                * VMEMMAP, early sections only exist when actually online.
> +                */
> +               if (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) &&
> +                   pfn >= next_chunk_pfn) {
> +                       is_online = !!pfn_to_online_page(pfn);
> +                       next_chunk_pfn = min(pfn + chunk, epfn);
> +               }
> +               if (is_online)
> +                       online_pgcnt++;
> +

It's hard to get it any cleaner. I was wondering whether we should just have a separate
for_each_online_pfn() call afterwards, but that's also not ideal.

I was wondering whether we should also just use pfn_section_valid() directly.

Something like the following:


diff --git a/mm/mm_init.c b/mm/mm_init.c
index e9c4204b73adb..cc7d55a71a060 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -838,22 +838,39 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  *   zone/node above the hole except for the trailing pages in the last
  *   section that will be appended to the zone/node below.
  */
-static void __init init_unavailable_range(unsigned long spfn,
-                                         unsigned long epfn,
-                                         int zone, int node)
+static unsigned long __init init_unavailable_range(unsigned long spfn,
+                                                  unsigned long epfn,
+                                                  int zone, int node)
 {
+       unsigned long next_chunk_pfn __maybe_unused = spfn;
        unsigned long pfn;
-       u64 pgcnt = 0;
+       u64 online_pgcnt = 0, pgcnt = 0;
+       bool is_online = true;
 
        for_each_valid_pfn(pfn, spfn, epfn) {
                __init_single_page(pfn_to_page(pfn), pfn, zone, node);
                __SetPageReserved(pfn_to_page(pfn));
                pgcnt++;
+
+               /*
+                * With vmemmap, at this stage all pages in an early section
+                * have a valid memmap and are marked as online. However, only
+                * subsections in the subsection map are actually online.
+                */
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
+               if (pfn >= next_chunk_pfn) {
+                       is_online = pfn_section_valid(__pfn_to_section(pfn), pfn);
+                       next_chunk_pfn = min(SUBSECTION_ALIGN_UP(pfn + 1), epfn);
+               }
+#endif
+               if (is_online)
+                       online_pgcnt++;
        }
 
        if (pgcnt)
                pr_info("On node %d, zone %s: %lld pages in unavailable ranges\n",
                        node, zone_names[zone], pgcnt);
+       return online_pgcnt;
 }
 
 /*


I'd love to avoid any CONFIG_SPARSEMEM_VMEMMAP, but that's hard as well :)

-- 
Cheers,

David