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.

-- 
Cheers,

David