Re: [PATCH v6 2/2] mm/memory_hotplug: improve shrink_zone_span() subsection boundary checks

[David Hildenbrand (Arm)]

On 8/7/26 05:22, Wei Yang wrote:
> On Wed, Aug 05, 2026 at 01:02:29PM +0200, David Hildenbrand (Arm) wrote:
>> On 7/23/26 10:49, Yuan Liu wrote:
>>> When shrinking a zone span after removing a PFN range,
>>> find_smallest_section_pfn() and find_biggest_section_pfn()
>>> only checked one edge PFN in each subsection for nid/zone matching.
>>>
>>> If a memory or hole boundary falls in the middle of a subsection,
>>> that edge PFN may belong to a different nid/zone, causing the helpers
>>> to miss a valid PFN within that subsection.
>>>
>>> Fix this by checking both subsection edge PFNs for nid/zone matching.
>>> Keep a single pfn_to_online_page() check per subsection, since online
>>> state is the same for all PFNs in a subsection.
>>>
>>> Reviewed-by: Jason Zeng <jason.zeng@xxxxxxxxx>
>>> Signed-off-by: Yuan Liu <yuan1.liu@xxxxxxxxx>
>>> ---
>>>  mm/memory_hotplug.c | 42 +++++++++++++++++++++++++++---------------
>>>  1 file changed, 27 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 4c699fd9c479..3a281d595207 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -427,17 +427,24 @@ static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
>>>  				     unsigned long start_pfn,
>>>  				     unsigned long end_pfn)
>>>  {
>>> -	for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
>>> -		if (unlikely(!pfn_to_online_page(start_pfn)))
>>> -			continue;
>>> +	unsigned long next_pfn;
>>>  
>>> -		if (unlikely(pfn_to_nid(start_pfn) != nid))
>>> -			continue;
>>> +	for (; start_pfn < end_pfn; start_pfn = next_pfn) {
>>> +		unsigned long tail_pfn;
>>>  
>>> -		if (zone != page_zone(pfn_to_page(start_pfn)))
>>> +		next_pfn = start_pfn + PAGES_PER_SUBSECTION;
>>> +		tail_pfn = next_pfn - 1;
>>> +
>>> +		if (unlikely(!pfn_to_online_page(start_pfn)))
>>>  			continue;
>>>  
>>> -		return start_pfn;
>>> +		if (likely(pfn_to_nid(start_pfn) == nid) &&
>>> +		    zone == page_zone(pfn_to_page(start_pfn)))
>>> +			return start_pfn;
>>> +
>>> +		if (likely(pfn_to_nid(tail_pfn) == nid) &&
>>> +		    zone == page_zone(pfn_to_page(tail_pfn)))
>>> +			return start_pfn;
>>>  	}
>>>  
>>>  	return 0;
>>> @@ -448,21 +455,26 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
>>>  				    unsigned long start_pfn,
>>>  				    unsigned long end_pfn)
>>>  {
>>> -	unsigned long pfn;
>>> +	unsigned long pfn, prev_pfn;
>>>  
>>>  	/* pfn is the end pfn of a memory section. */
>>>  	pfn = end_pfn - 1;
>>> -	for (; pfn >= start_pfn; pfn -= PAGES_PER_SUBSECTION) {
>>> -		if (unlikely(!pfn_to_online_page(pfn)))
>>> -			continue;
>>> +	for (; pfn >= start_pfn; pfn = prev_pfn) {
>>> +		unsigned long head_pfn;
>>>  
>>> -		if (unlikely(pfn_to_nid(pfn) != nid))
>>> -			continue;
>>> +		prev_pfn = pfn - PAGES_PER_SUBSECTION;
>>> +		head_pfn = prev_pfn + 1;
>>>  
>>> -		if (zone != page_zone(pfn_to_page(pfn)))
>>> +		if (unlikely(!pfn_to_online_page(pfn)))
>>>  			continue;
>>>  
>>> -		return pfn;
>>> +		if (likely(pfn_to_nid(pfn) == nid) &&
>>> +		    zone == page_zone(pfn_to_page(pfn)))
>>> +			return pfn;
>>> +
>>> +		if (likely(pfn_to_nid(head_pfn) == nid) &&
>>> +		    zone == page_zone(pfn_to_page(head_pfn)))
>>> +			return pfn;
>>>  	}
>>>  
>>>  	return 0;
>>
>>
>> I think improving that should be patch #1.
>>
>> But I think we can do much better code-wise. What about the following
>> cleanup instead:
>>
>>
>> >From 6e9ea094daf8d04c0b119b780572a846334f8164 Mon Sep 17 00:00:00 2001
>> From: "David Hildenbrand (Arm)" <david@xxxxxxxxxx>
>> Date: Wed, 5 Aug 2026 12:52:55 +0200
>> Subject: [PATCH] mm/memory_hotplug: make shrink_zone_span() more robust
>>
>> Let's make shrink_zone_span() more robust by checking in
>> find_smallest_section_pfn() / find_biggest_section_pfn() that start+end
>> of the subsection.
>>
>> While at it, clean up the function heavily, factoring the core check
>> out into subsection_overlaps_zone().
>>
>> There likely is no need to check for the nid first: we require
>> SPARSEMEM_VMEMMAP_ENABLE where pfn_to_page() is cheap, and the
>> pfn_to_nid() on CONFIG_NUMA would do a pfn_to_page() either way. So
>> let's just drop that for now.
>>
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> ---
>> mm/memory_hotplug.c | 59 ++++++++++++++++++---------------------------
>> 1 file changed, 24 insertions(+), 35 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 226ab9cb078ad..79937f9672d01 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -425,49 +425,39 @@ int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> 	return err;
>> }
>>
>> -/* find the smallest valid pfn in the range [start_pfn, end_pfn) */
>> -static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
>> -				     unsigned long start_pfn,
>> -				     unsigned long end_pfn)
>> +static bool subsection_overlaps_zone(unsigned long pfn, struct zone *zone)
>> {
>> -	for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
>> -		if (unlikely(!pfn_to_online_page(start_pfn)))
>> -			continue;
>> +	const unsigned long start_pfn = ALIGN_DOWN(pfn, PAGES_PER_SUBSECTION);
>> +	const unsigned long end_pfn = start_pfn + PAGES_PER_SUBSECTION - 1;
>>
>> -		if (unlikely(pfn_to_nid(start_pfn) != nid))
>> -			continue;
>> +	/* All pages in a subsection are either online or offline. */
>> +	if (unlikely(!pfn_to_online_page(start_pfn)))
>> +		return false;
>>
>> -		if (zone != page_zone(pfn_to_page(start_pfn)))
>> -			continue;
>> +	/* Checking start+end is sufficient. */
>> +	return zone == page_zone(pfn_to_page(start_pfn)) ||
>> +	       zone == page_zone(pfn_to_page(end_pfn));
>> +}
> 
> Hi, David

Hi,

> 
> I am thinking if we could initialize each subsection to only one zone, we may
> reduce the complexity?
> 
> For example, in init_unavailable_range() align spfn to subsection and init the
> head unaligned part to previous zone?
> 
> Do you think this is reasonable?

I'm afraid of opening up another can of worms. :)

The additional page_zone(pfn_to_page(end_pfn) check is cheap with VMEMMAP, so I
think we should focus on that first.

(and the patch is in general a cleanup :) )

-- 
Cheers,

David