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

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 <[email protected]>
>> Signed-off-by: Yuan Liu <[email protected]>
>> ---
>>  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)" <[email protected]>
>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) <[email protected]>
>---
> 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

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?

> 
>-		return start_pfn;
>+/* find the smallest valid pfn in the range [start_pfn, end_pfn) */
>+static unsigned long find_smallest_section_pfn(struct zone *zone,
>+		unsigned long start_pfn, unsigned long end_pfn)
>+{
>+	for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
>+		if (subsection_overlaps_zone(start_pfn, zone))
>+			return start_pfn;
> 	}
>-
> 	return 0;
> }
> 
> /* find the biggest valid pfn in the range [start_pfn, end_pfn). */
>-static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
>-				    unsigned long start_pfn,
>-				    unsigned long end_pfn)
>+static unsigned long find_biggest_section_pfn(struct zone *zone,
>+		unsigned long start_pfn, unsigned long end_pfn)
> {
>-	unsigned long 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;
>-
>-		if (unlikely(pfn_to_nid(pfn) != nid))
>-			continue;
>-
>-		if (zone != page_zone(pfn_to_page(pfn)))
>-			continue;
>-
>-		return pfn;
>+	for (; end_pfn >= start_pfn; end_pfn -= PAGES_PER_SUBSECTION) {
>+		if (subsection_overlaps_zone(end_pfn - 1, zone))
>+			return end_pfn - 1;
> 	}
>-
> 	return 0;
> }
> 
>@@ -475,7 +465,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> 			     unsigned long end_pfn)
> {
> 	unsigned long pfn;
>-	int nid = zone_to_nid(zone);
> 
> 	if (zone->zone_start_pfn == start_pfn) {
> 		/*
>@@ -484,7 +473,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> 		 * In this case, we find second smallest valid mem_section
> 		 * for shrinking zone.
> 		 */
>-		pfn = find_smallest_section_pfn(nid, zone, end_pfn,
>+		pfn = find_smallest_section_pfn(zone, end_pfn,
> 						zone_end_pfn(zone));
> 		if (pfn) {
> 			zone->spanned_pages = zone_end_pfn(zone) - pfn;
>@@ -500,7 +489,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> 		 * In this case, we find second biggest valid mem_section for
> 		 * shrinking zone.
> 		 */
>-		pfn = find_biggest_section_pfn(nid, zone, zone->zone_start_pfn,
>+		pfn = find_biggest_section_pfn(zone, zone->zone_start_pfn,
> 					       start_pfn);
> 		if (pfn)
> 			zone->spanned_pages = pfn - zone->zone_start_pfn + 1;
>-- 
>2.43.0
>
>
>-- 
>Cheers,
>
>David

-- 
Wei Yang
Help you, Help me