Re: [PATCH v2 2/4] mm/page_isolation: remove migratetype from move_freepages_block_isolate()

[Zi Yan <ziy@nvidia.com>]

On 14 Feb 2025, at 12:20, Johannes Weiner wrote:

> On Fri, Feb 14, 2025 at 10:42:13AM -0500, Zi Yan wrote:
>> Since migratetype is no longer overwritten during pageblock isolation,
>> moving pageblocks to and from MIGRATE_ISOLATE do not need migratetype.
>>
>> Rename move_freepages_block_isolate() to share common code and add
>> pageblock_isolate_and_move_free_pages() and
>> pageblock_unisolate_and_move_free_pages() to be explicit about the page
>> isolation operations.
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>  include/linux/page-isolation.h |  4 +--
>>  mm/page_alloc.c                | 48 +++++++++++++++++++++++++++-------
>>  mm/page_isolation.c            | 21 +++++++--------
>>  3 files changed, 51 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
>> index 51797dc39cbc..28c56f423e34 100644
>> --- a/include/linux/page-isolation.h
>> +++ b/include/linux/page-isolation.h
>> @@ -27,8 +27,8 @@ static inline bool is_migrate_isolate(int migratetype)
>>
>>  void set_pageblock_migratetype(struct page *page, int migratetype);
>>
>> -bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>> -				  int migratetype);
>> +bool pageblock_isolate_and_move_free_pages(struct zone *zone, struct page *page);
>> +bool pageblock_unisolate_and_move_free_pages(struct zone *zone, struct page *page);
>>
>>  int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>>  			     int migratetype, int flags);
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index f17f4acc38c6..9bba5b1c4f1d 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1848,10 +1848,10 @@ static unsigned long find_large_buddy(unsigned long start_pfn)
>>  }
>>
>>  /**
>> - * move_freepages_block_isolate - move free pages in block for page isolation
>> + * __move_freepages_for_page_isolation - move free pages in block for page isolation
>>   * @zone: the zone
>>   * @page: the pageblock page
>> - * @migratetype: migratetype to set on the pageblock
>> + * @isolate_pageblock to isolate the given pageblock or unisolate it
>>   *
>>   * This is similar to move_freepages_block(), but handles the special
>>   * case encountered in page isolation, where the block of interest
>> @@ -1866,10 +1866,15 @@ static unsigned long find_large_buddy(unsigned long start_pfn)
>>   *
>>   * Returns %true if pages could be moved, %false otherwise.
>>   */
>> -bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>> -				  int migratetype)
>> +static bool __move_freepages_for_page_isolation(struct zone *zone,
>> +		struct page *page, bool isolate_pageblock)
>
> I'm biased, but I think the old name is fine.
>
> bool isolate?

OK. Will use the old name and bool isolate.
>
>>  {
>>  	unsigned long start_pfn, pfn;
>> +	int from_mt;
>> +	int to_mt;
>> +
>> +	if (isolate_pageblock == get_pageblock_isolate(page))
>> +		return false;
>>
>>  	if (!prep_move_freepages_block(zone, page, &start_pfn, NULL, NULL))
>>  		return false;
>> @@ -1886,7 +1891,10 @@ bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>>
>>  		del_page_from_free_list(buddy, zone, order,
>>  					get_pfnblock_migratetype(buddy, pfn));
>> -		set_pageblock_migratetype(page, migratetype);
>> +		if (isolate_pageblock)
>> +			set_pageblock_isolate(page);
>> +		else
>> +			clear_pageblock_isolate(page);
>
> Since this pattern repeats, maybe create a toggle function that does this?
>
> 		set_pfnblock_flags_mask(page, (isolate << PB_migrate_isolate),
> 					page_to_pfn(page),
> 					(1 << PB_migrate_isolate));

Sure.

>
>>  		split_large_buddy(zone, buddy, pfn, order, FPI_NONE);
>>  		return true;
>>  	}
>> @@ -1897,16 +1905,38 @@ bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>>
>>  		del_page_from_free_list(page, zone, order,
>>  					get_pfnblock_migratetype(page, pfn));
>> -		set_pageblock_migratetype(page, migratetype);
>> +		if (isolate_pageblock)
>> +			set_pageblock_isolate(page);
>> +		else
>> +			clear_pageblock_isolate(page);
>>  		split_large_buddy(zone, page, pfn, order, FPI_NONE);
>>  		return true;
>>  	}
>>  move:
>> -	__move_freepages_block(zone, start_pfn,
>> -			       get_pfnblock_migratetype(page, start_pfn),
>> -			       migratetype);
>> +	if (isolate_pageblock) {
>> +		from_mt = __get_pfnblock_flags_mask(page, page_to_pfn(page),
>> +				MIGRATETYPE_MASK);
>> +		to_mt = MIGRATE_ISOLATE;
>> +	} else {
>> +		from_mt = MIGRATE_ISOLATE;
>> +		to_mt = __get_pfnblock_flags_mask(page, page_to_pfn(page),
>> +				MIGRATETYPE_MASK);
>> +	}
>> +
>> +	__move_freepages_block(zone, start_pfn, from_mt, to_mt);
>>  	return true;
>
> Keeping both MIGRATE_ISOLATE and PB_migrate_isolate encoding the same
> state is fragile. At least in the pfnblock bitmask, there should only
> be one bit encoding this.
>
> Obviously, there are many places in the allocator that care about
> freelist destination: they want MIGRATE_ISOLATE if the bit is set, and
> the "actual" type otherwise.
>
> I think what should work is decoupling enum migratetype from the
> pageblock bits, and then have a parsing layer on top like this:
>
> enum migratetype {
> 	MIGRATE_UNMOVABLE,
> 	...
> 	MIGRATE_TYPE_BITS,
> 	MIGRATE_ISOLATE = MIGRATE_TYPE_BITS,
> 	MIGRATE_TYPES
> };
>
> #define PB_migratetype_bits MIGRATE_TYPE_BITS
>
> static enum migratetype get_pageblock_migratetype()
> {
> 	flags = get_pfnblock_flags_mask(..., MIGRATETYPE_MASK | (1 << PB_migrate_isolate));
> 	if (flags & (1 << PB_migrate_isolate))
> 		return MIGRATE_ISOLATE;
> 	return flags;
> }
I had something similar in RFC and change to current implementation
to hide the details. But that is fragile like you said. I will try
your approach in the next version.

Thank you for the reviews. :)

Best Regards,
Yan, Zi