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

["Li, Tianyou" <tianyou.li@intel.com>]

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.