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

[Liu, Yuan1]

> -----Original Message-----
> From: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Sent: Wednesday, August 5, 2026 7:53 PM
> 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>; 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 1/2] mm/memory_hotplug: optimize zone contiguous
> check when changing pfn range
>

[...]

> >
> > diff --git a/Documentation/mm/physical_memory.rst
> b/Documentation/mm/physical_memory.rst
> > index b76183545e5b..0aa65e6b5499 100644
> > --- a/Documentation/mm/physical_memory.rst
> > +++ b/Documentation/mm/physical_memory.rst
> > @@ -483,6 +483,19 @@ General
> >    ``present_pages`` should use ``get_online_mems()`` to get a stable
> value. It
> >    is initialized by ``calculate_node_totalpages()``.
> >
> > +``pages_with_online_memmap``
> > +  Tracks pages within the zone that have an online memory map (present
> pages
> > +  and memory holes whose memory map has been initialized). When
> > +  ``spanned_pages`` == ``pages_with_online_memmap``, ``pfn_to_page()``
> can be
> > +  performed without further checks on any PFN within the zone span.
> > +
> > +  Note: this counter may temporarily undercount when pages with an
> online
> > +  memory map exist outside the current zone span. This can only happen
> during
> > +  boot, when initializing the memory map of pages that do not fall into
> any
> > +  zone span. Growing the zone to cover such pages and later shrinking
> it back
> > +  may result in a "too small" value. This is safe: it merely prevents
> > +  detecting a contiguous zone.
> 
> It's suboptimal that we repeat the same comment that we already have in
> struct
> zone. Can we just keep it vry simple here?
> 
> "Pages within the zone that have an online memory map: present pages and
> memory
> holes whose memory map has been initialized. See XXX for more details."

Thanks for pointing out, I will simplify to this short description.

> > +
> >  ``present_early_pages``
> >    The present pages existing within the zone located on memory
> available since
> >    early boot, excluding hotplugged memory. Defined only when
> > diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> > index bcfe2d9f4adb..237ace435372 100644
> > --- a/drivers/base/memory.c
> > +++ b/drivers/base/memory.c
> > @@ -246,6 +246,7 @@ static int memory_block_online(struct memory_block
> *mem)
> >  		nr_vmemmap_pages = mem->altmap->free;
> >
> >  	mem_hotplug_begin();
> > +	clear_zone_contiguous(zone);
> >  	if (nr_vmemmap_pages) {
> >  		ret = mhp_init_memmap_on_memory(start_pfn, nr_vmemmap_pages,
> zone);
> >  		if (ret)
> > @@ -270,6 +271,7 @@ static int memory_block_online(struct memory_block
> *mem)
> >
> >  	mem->zone = zone;
> >  out:
> > +	set_zone_contiguous(zone);
> >  	mem_hotplug_done();
> >  	return ret;
> >  }
> > @@ -282,6 +284,7 @@ static int memory_block_offline(struct memory_block
> *mem)
> >  	unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr);
> >  	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
> >  	unsigned long nr_vmemmap_pages = 0;
> > +	struct zone *zone;
> 
> Why the temporary variable, and why not initialize it directly here? Note
> that
> 
> >  	int ret;
> >
> >  	if (!mem->zone)
> 
> We already use mem->zone here. So if you add a variable, convert that one
> as
> well. But I guess we can just life without one.

Will remove the temporary variable in next version.

> > @@ -294,7 +297,9 @@ static int memory_block_offline(struct memory_block
> *mem)
> >  	if (mem->altmap)
> >  		nr_vmemmap_pages = mem->altmap->free;
> >
> > +	zone = mem->zone;
> >  	mem_hotplug_begin();
> > +	clear_zone_contiguous(zone);
> >  	if (nr_vmemmap_pages)
> >  		adjust_present_page_count(pfn_to_page(start_pfn), mem->group,
> >  					  -nr_vmemmap_pages);
> > @@ -314,6 +319,7 @@ static int memory_block_offline(struct memory_block
> *mem)
> >
> >  	mem->zone = NULL;
> >  out:
> > +	set_zone_contiguous(zone);
> >  	mem_hotplug_done();
> >  	return ret;
> >  }
> 
> [...]
> 
> > +static inline void set_zone_contiguous(struct zone *zone)
> > +{
> > +	if (zone_is_zone_device(zone))
> > +		return;
> > +	if (zone->spanned_pages == zone->pages_with_online_memmap)
> > +		zone->contiguous = true;
> 
> Maybe it was already discussed (and I recall that we previously had that),
> but I
> think we really need READ_ONCE semantics here and WRITE_ONCE semantics in
> memory
> hotplug code. Otherwise concurrent updates could lead to weird things when
> the
> compiler does load-tearing.

Thanks for your guidance. I should have considered the
READ_ONCE/WRITE_ONCE semantics here. I'll add them in the next
Revision.

> [...]
> 
> >
> > +static void __init update_zone_online_memmap_pages(struct zone *zone,
> > +						   unsigned long start_pfn,
> > +						   unsigned long end_pfn,
> > +						   unsigned long *hole_pfn)
> > +{
> > +#ifdef CONFIG_SPARSEMEM_VMEMMAP
> > +	unsigned long zone_start_pfn = zone->zone_start_pfn;
> > +	unsigned long zone_end_pfn = zone_start_pfn + zone->spanned_pages;
> 
> These two can be const.

Will do.

> > +	unsigned long sub_start, sub_end;
> > +
> > +	sub_start = max(ALIGN_DOWN(start_pfn, PAGES_PER_SUBSECTION),
> > +			zone_start_pfn);
> > +	sub_end = min(ALIGN(end_pfn, PAGES_PER_SUBSECTION), zone_end_pfn);
> 
> Hm, I don't immediately understand why we do the PAGES_PER_SUBSECTION
> thing
> here. Why is that required?

pages_with_online_memmap counts all PFNs where pfn_to_online_page() is
valid. With CONFIG_SPARSEMEM_VMEMMAP, pfn_section_valid() operates at
PAGES_PER_SUBSECTION granularity — when any page in a subsection has
memory, the entire subsection is valid/online. So we align to subsection
boundaries to include hole pages within partially-populated subsections.

I can also add a comment in the code to make this intent clearer.
/*
 * With CONFIG_SPARSEMEM_VMEMMAP, pfn_section_valid() operates at
 * PAGES_PER_SUBSECTION granularity, so align to subsection boundaries
 * to include all PFNs for which pfn_to_online_page() is valid.
 */

Best Regards,
Liu, Yuan1

> Cheers,
> 
> David