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

[Liu, Yuan1]

> -----Original Message-----
> From: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Sent: Friday, August 21, 2026 1:46 AM
> To: Liu, Yuan1 <yuan1.liu@xxxxxxxxx>; Oscar Salvador <osalvador@xxxxxxx>;
> Mike Rapoport <rppt@xxxxxxxxxx>; Wei Yang <richard.weiyang@xxxxxxxxx>
> Cc: linux-mm@xxxxxxxxx; Zou, Nanhai <nanhai.zou@xxxxxxxxx>; Chen Zhang
> <zhangchen.kidd@xxxxxx>; Zeng, Jason <jason.zeng@xxxxxxxxx>; Chen, Yu C
> <yu.c.chen@xxxxxxxxx>; Deng, Pan <pan.deng@xxxxxxxxx>; Li, Tianyou
> <tianyou.li@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 2/2] mm/memory_hotplug: optimize zone contiguous
> check when changing pfn range
> 
> >>> +			online_pgcnt++;
> >>
> >> Can we avoid these helpers?
> >>
> >> 		const unsigned long subsection = pfn & PAGE_SUBSECTION_MASK;
> >>
> >> 		/* We can have section-sized online holes with VMEMMAP. */
> >> 		if (IS_ENMABLED(CONFIG_SPARSEMEM_VMEMMAP) &&
> >> 		    subsection != last_subsection) {
> >> 			is_online = pfn_to_online_page(pfn);
> >> 			last_subsection = subsection;
> >> 		}
> >> 		if (is_online)
> >> 			online_pgcnt++;
> >
> > 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.
> 
> >
> >> An alternative is an inner loop that just walks in SUBSECTION chunks
> until
> >> epfn.
> >> That would probably be even cleaner and faster.
> >>
> >> I remember !vmemmap always only has early sections when they are
> actually
> >> online. We could extent the comment to clarify that.
> >
> > 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++;
+

Best Regards,
Liu, Yuan

> Cheers,
> 
> David