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

[Li, Tianyou]

On 12/2/2025 6:57 PM, Mike Rapoport wrote:
> 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.
>   


Thanks Mike. It's doable, consider clear_zone_contiguous moved out.

> > > +	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.


Got it. Thanks.


> > > +		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.
>   


Got it. Thanks.