Re: [PATCH v4] mm/memory hotplug/unplug: Optimize zone->contiguous update when changes pfn range

[Mike Rapoport <rppt@kernel.org>]

On Mon, Dec 01, 2025 at 07:54:34PM +0100, David Hildenbrand (Red Hat) wrote:
> On 12/1/25 14:22, Tianyou Li wrote:

...

> >   void remove_pfn_range_from_zone(struct zone *zone,
> >   				      unsigned long start_pfn,
> >   				      unsigned long nr_pages)
> > @@ -551,6 +577,7 @@ void remove_pfn_range_from_zone(struct zone *zone,
> >   	const unsigned long end_pfn = start_pfn + nr_pages;
> >   	struct pglist_data *pgdat = zone->zone_pgdat;
> >   	unsigned long pfn, cur_nr_pages;
> > +	enum zone_contiguous_state contiguous_state = CONTIGUOUS_UNDETERMINED;
> >   	/* Poison struct pages because they are now uninitialized again. */
> >   	for (pfn = start_pfn; pfn < end_pfn; pfn += cur_nr_pages) {
> > @@ -571,12 +598,13 @@ void remove_pfn_range_from_zone(struct zone *zone,
> >   	if (zone_is_zone_device(zone))
> >   		return;
> > -	clear_zone_contiguous(zone);
> > +	contiguous_state = clear_zone_contiguous_for_shrinking(
> > +				zone, start_pfn, nr_pages);
> 
> Reading this again, I wonder whether it would be nicer to have something
> like:
> 
> new_contig_state = zone_contig_state_after_shrinking();
> clear_zone_contiguous(zone);

+1

> > +static enum zone_contiguous_state __meminit clear_zone_contiguous_for_growing(
> > +		struct zone *zone, unsigned long start_pfn, unsigned long nr_pages)
> > +{
> > +	const unsigned long end_pfn = start_pfn + nr_pages;
> > +	enum zone_contiguous_state result = CONTIGUOUS_UNDETERMINED;
> > +
> > +	/*
> > +	 * Given the moved pfn range's contiguous property is always true,
> > +	 * under the conditional of empty zone, the contiguous property should
> > +	 * be true.
> > +	 */
> 
> I don't think that comment is required.
> 
> > +	if (zone_is_empty(zone))
> > +		result = CONTIGUOUS_DEFINITELY;
> > +
> > +	/*
> > +	 * If the moved pfn range does not intersect with the original zone span,
> > +	 * the contiguous property is surely false.
> > +	 */
> > +	else if (end_pfn < zone->zone_start_pfn || start_pfn > zone_end_pfn(zone))
> > +		result = CONTIGUOUS_DEFINITELY_NOT;
> > +
> > +	/*
> > +	 * If the moved pfn range is adjacent to the original zone span, given
> > +	 * the moved pfn range's contiguous property is always true, the zone's
> > +	 * contiguous property inherited from the original value.
> > +	 */
> > +	else if (end_pfn == zone->zone_start_pfn || start_pfn == zone_end_pfn(zone))
> > +		result = zone->contiguous ?
> > +			CONTIGUOUS_DEFINITELY : CONTIGUOUS_DEFINITELY_NOT;
> > +
> > +	/*
> > +	 * If the original zone's hole larger than the moved pages in the range,
> > +	 * the contiguous property is surely false.
> > +	 */
> > +	else if (nr_pages < (zone->spanned_pages - zone->present_pages))
> > +		result = CONTIGUOUS_DEFINITELY_NOT;
> > +
> 
> This is a bit unreadable :)
> 
> if (zone_is_empty(zone)) {
> 	result = CONTIGUOUS_DEFINITELY;
> } else if (...) {
> 	/* ... */
> 	...
> } else if (...) {
> 	...
> }

If we update zone state outside this function it can be even simpler:

	if (zone_is_empty(zone))
		return CONTIGUOUS_DEFINITELY;

	if (nr_pages < (zone->spanned_pages - zone->present_pages))
		return CONTIGUOUS_DEFINITELY_NOT;

etc.
 
> > +	clear_zone_contiguous(zone);
> > +	return result;
> > +}
> > +
> >   /*
> >    * Associate the pfn range with the given zone, initializing the memmaps
> >    * and resizing the pgdat/zone data to span the added pages. After this
> > @@ -752,8 +821,8 @@ void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
> >   {
> >   	struct pglist_data *pgdat = zone->zone_pgdat;
> >   	int nid = pgdat->node_id;
> > -
> > -	clear_zone_contiguous(zone);
> > +	const enum zone_contiguous_state contiguous_state =
> > +		clear_zone_contiguous_for_growing(zone, start_pfn, nr_pages);
> >   	if (zone_is_empty(zone))
> >   		init_currently_empty_zone(zone, start_pfn, nr_pages);
> > @@ -783,7 +852,7 @@ void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
> >   			 MEMINIT_HOTPLUG, altmap, migratetype,
> >   			 isolate_pageblock);
> > -	set_zone_contiguous(zone);
> > +	set_zone_contiguous(zone, contiguous_state);
> >   }
> >   struct auto_movable_stats {
> > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > index 7712d887b696..06db3fcf7f95 100644
> > --- a/mm/mm_init.c
> > +++ b/mm/mm_init.c
> > @@ -2263,26 +2263,34 @@ void __init init_cma_pageblock(struct page *page)
> >   }
> >   #endif
> > -void set_zone_contiguous(struct zone *zone)
> > +void set_zone_contiguous(struct zone *zone, enum zone_contiguous_state state)
> >   {
> >   	unsigned long block_start_pfn = zone->zone_start_pfn;
> >   	unsigned long block_end_pfn;
> > -	block_end_pfn = pageblock_end_pfn(block_start_pfn);
> > -	for (; block_start_pfn < zone_end_pfn(zone);
> > -			block_start_pfn = block_end_pfn,
> > -			 block_end_pfn += pageblock_nr_pages) {
> > +	if (state == CONTIGUOUS_DEFINITELY) {
> > +		zone->contiguous = true;
> > +		return;
> > +	} else if (state == CONTIGUOUS_DEFINITELY_NOT) {
> > +		// zone contiguous has already cleared as false, just return.

Please no C++ style comments.

> > +		return;
> > +	} else if (state == CONTIGUOUS_UNDETERMINED) {
> > +		block_end_pfn = pageblock_end_pfn(block_start_pfn);
> > +		for (; block_start_pfn < zone_end_pfn(zone);
> > +				block_start_pfn = block_end_pfn,
> > +				block_end_pfn += pageblock_nr_pages) {
> > -		block_end_pfn = min(block_end_pfn, zone_end_pfn(zone));
> > +			block_end_pfn = min(block_end_pfn, zone_end_pfn(zone));
> > -		if (!__pageblock_pfn_to_page(block_start_pfn,
> > -					     block_end_pfn, zone))
> > -			return;
> > -		cond_resched();
> > -	}
> > +			if (!__pageblock_pfn_to_page(block_start_pfn,
> > +						block_end_pfn, zone))
> > +				return;
> > +			cond_resched();
> > +		}
> > -	/* We confirm that there is no hole */
> > -	zone->contiguous = true;
> > +		/* We confirm that there is no hole */
> > +		zone->contiguous = true;
> > +	}
> >   }
> 
> 
> switch (state) {
> case CONTIGUOUS_DEFINITELY:
> 	zone->contiguous = true;
> 	return;
> case CONTIGUOUS_DEFINITELY_NOT:
> 	return;
> default:
> 	break;
> }
> ... unchanged logic.

I was going to suggest rather to drop 'else' 

	if (state == CONTIGUOUS_DEFINITELY) {
		zone->contiguous = true;
		return;
	}

	if (state == CONTIGUOUS_DEFINITELY_NOT) 
		return;

	... unchanged logic.

but I don't feel strongly about it.
 
-- 
Sincerely yours,
Mike.