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

[Wei Yang]

On Thu, Jul 30, 2026 at 07:57:13AM +0000, Liu, Yuan1 wrote:
>> -----Original Message-----
>> From: Wei Yang <richard.weiyang@xxxxxxxxx>
>> Sent: Thursday, July 30, 2026 10:36 AM
>> To: Liu, Yuan1 <yuan1.liu@xxxxxxxxx>
>> Cc: Wei Yang <richard.weiyang@xxxxxxxxx>; David Hildenbrand
>> <david@xxxxxxxxxx>; Oscar Salvador <osalvador@xxxxxxx>; Mike Rapoport
>> <rppt@xxxxxxxxxx>; linux-mm@xxxxxxxxx; Zou, Nanhai <nanhai.zou@xxxxxxxxx>;
>> Deng, Pan <pan.deng@xxxxxxxxx>; Li, Tianyou <tianyou.li@xxxxxxxxx>; Chen
>> Zhang <zhangchen.kidd@xxxxxx>; Zeng, Jason <jason.zeng@xxxxxxxxx>; linux-
>> kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v6 2/2] mm/memory_hotplug: improve shrink_zone_span()
>> subsection boundary checks
>> 
>> On Mon, Jul 27, 2026 at 09:49:39AM +0000, Liu, Yuan1 wrote:
>> >> -----Original Message-----
>> >> From: Wei Yang <richard.weiyang@xxxxxxxxx>
>> >> Sent: Saturday, July 25, 2026 10:50 AM
>> >> To: Liu, Yuan1 <yuan1.liu@xxxxxxxxx>
>> >> Cc: David Hildenbrand <david@xxxxxxxxxx>; Oscar Salvador
>> >> <osalvador@xxxxxxx>; Mike Rapoport <rppt@xxxxxxxxxx>; Wei Yang
>> >> <richard.weiyang@xxxxxxxxx>; linux-mm@xxxxxxxxx; Zou, Nanhai
>> >> <nanhai.zou@xxxxxxxxx>; Deng, Pan <pan.deng@xxxxxxxxx>; Li, Tianyou
>> >> <tianyou.li@xxxxxxxxx>; Chen Zhang <zhangchen.kidd@xxxxxx>; Zeng, Jason
>> >> <jason.zeng@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
>> >> Subject: Re: [PATCH v6 2/2] mm/memory_hotplug: improve
>> shrink_zone_span()
>> >> subsection boundary checks
>> >>
>> >> On Thu, Jul 23, 2026 at 04:49:46AM -0400, 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;
>> >>
>> >> Here we are checking range [start_pfn, tail_pfn]. When we come here, it
>> >> means
>> >> start_pfn's nid or zone doesn't match our expectation. But if tail_pfn
>> >> does,
>> >> why it still return start_pfn?
>> >
>> >Hi Wei
>> >
>> >If start_pfn falls into a hole while tail_pfn still belongs to a valid
>> >memblock in this zone, skipping the subsection would cause
>> >shrink_zone_span() to shrink the zone span too aggressively, excluding
>> >valid PFNs from the zone.
>> >
>> >Since init_unavailable_range() initializes hole pages with the correct
>> >zone/nid, the start_pfn check always succeeds here, making the tail_pfn
>> >check redundant today.
>> >
>> >That said, I wonder if it is still worth keeping this check so that
>> >shrink_zone_span() does not depend on how hole pages are initialized.
>> >
>> 
>> Looks reasonable.
>> 
>> I search the discussion history, and found David suggest this fix in [1]
>> with
>> following statement.
>> 
>>   Well, unless we have an odd case where the hole+memory starts in the
>>   middle of a "PAGES_PER_SUBSECTION". That would already be problematic if
>>   memory starts/ends in the middle of a PAGES_PER_SUBSECTION chunk. I
>>   don't such a case exists.
>> 
>>   We could improve shrink_zone_span() to let
>>   find_smallest_section_pfn/find_biggest_section_pfn test the pfn_to_nid()
>>   and page_zone() not on;y on the smallest/highest pfn, but also on the
>>   highest/smallest PFN in a PAGES_PER_SUBSECTION chunk.
>> 
>> I am trying to understand the exact case David described, but not fully
>> get
>> it. Would you mind describing more, so we would make sure not missing the
>> point.
>> 
>> [1]: https://lore.kernel.org/all/e86fee84-08d8-4563-8596-
>> e40d8e196799@xxxxxxxxxx/T/#u
>
>I am not sure whether this scenario can occur on real systems, but
>from reading the current code it seems there may be a corner case.
>
>For example, consider a zone whose tail ends with a single-PFN hole.
>init_unavailable_range() initializes this hole page and assigns its
>nid/zone to the adjacent next zone, since the PFN itself belongs outside 
>the current zone.
>
>When memory is hot-added after this hole, the hotplug granularity
>(PAGES_PER_SUBSECTION) extends the zone span to include the entire
>subsection containing the hole.
>

IIUC, for memory hotplug, the alignment is guaranteed by
check_hotplug_memory_range() with min block size which is section aligned.

>Later, when the hot-added memory is removed,
>find_biggest_section_pfn() examines the hole page first. Because its
>nid/zone belongs to the adjacent next zone, the boundary subsection may 
>be considered removable even though it still contains boot memory pages.
>
>This appears to be asymmetric between the zone head and the zone tail.
>
>At the zone head (find_smallest_section_pfn()), the boundary subsection
>has a hole + memory layout. The hole pages belong to the current zone,
>so the subsection is retained.
>
>At the zone tail (find_biggest_section_pfn()), the boundary subsection
>has a memory + hole layout. The trailing hole pages belong to the
>adjacent next zone, so the subsection may be discarded.
>

I am trying to see what the real case it would be.

Hmm.. First I don't expect there would be memory hotplug happens between zones
or nodes. So the new added memory range always sits after boot memory(early
sections).

Then let's assume below memory layout after bootup.

  |< Zone Normal  >|     |< Zone Movable >|
  +----------------+.....+----------------+.....+
  |                |hole1|                |hole2|
  +----------------+.....+----------------+.....+

Both hole is less than subsection size, and the hole2 expand to section
aligned address. According to the init_unavailable_range() during
memmap_init(), both holes are init to Zone Movable.

Then we can hot-add next section and hot-online it to:

  1) Zone Movable
  2) Zone Normal

For case 1), if we offline this section, we run into
find_biggest_section_pfn() for Zone Movable. It test the last pfn in hole2,
and see it meets the condition. So Zone Movable results in spanning hole2,
which is larger than original value. This is not that accurate, but still
could work.

For case 2), if we offline this section, we run into
find_biggest_section_pfn() for Zone Normal. Here we also have two cases based
on the location of the hole:

  a) hole starts at the beginning of subsection (hole + memory)
  b) hole ends at the end of subsection (memory + hole)

  For a), it looks good, because the subsection hole is treated as Zone
  Movable, and Zone Normal is correctly sized.
  
  For b), it seems not right, as we only test subsection's last pfn. As it
  belongs to Zone Movable we would skip the subsection and set Normal Zone
  span to previous subsection. Even some of the pfn belongs to Zone Normal.
  
  Hmm... I guess this is the case David mentioned.
  
  And with your patch, it checks head_pfn of this subsection, so it fixes this
  by extend Zone Normal to just ahead Zone Movable. This works something like
  a), but make Zone Normal span to some pfn initialized to Zone Movable.

Based on above analysis, let's see when find_smallest_section_pfn() will have
trouble.

The case in my mind is like below:

  |< Zone Normal  >|< Zone Movable                        >|
  +----------------+----------------+.....+----------------+
  |                |                |hole |                |
  +----------------+----------------+.....+----------------+

Hole should sits at the beginning of a section, otherwise offline_pages()
would prevent offline the range.  Then when offline those beginning Movable
memory, find_smallest_section_pfn() is triggered. But as hole is init to Zone
Movable, it looks good.

On summary:

  * find_biggest_section_pfn() may truncate zone span range if there is a
    hole less than subsection sits at the end of subsection
  * find_smallest_section_pfn() looks good even there is a hole less than
    subsection

Above is my understanding about the possible cases when re-sizing zone. In
case missing some point, just let me know :-)

-- 
Wei Yang
Help you, Help me