Re: [PATCH v5 4/6] mm/page_isolation: remove migratetype from move_freepages_block_isolate()

[Zi Yan <ziy@nvidia.com>]

On 27 May 2025, at 6:50, Vlastimil Babka wrote:

> On 5/23/25 21:12, Zi Yan wrote:
>> Since migratetype is no longer overwritten during pageblock isolation,
>> moving a pageblock out of MIGRATE_ISOLATE no longer needs a new
>> migratetype.
>>
>> Add pageblock_isolate_and_move_free_pages() and
>> pageblock_unisolate_and_move_free_pages() to be explicit about the page
>> isolation operations. Both share the common code in
>> __move_freepages_block_isolate(), which is renamed from
>> move_freepages_block_isolate().
>>
>> Add toggle_pageblock_isolate() to flip pageblock isolation bit in
>> __move_freepages_block_isolate().
>>
>> Make set_pageblock_migratetype() only accept non MIGRATE_ISOLATE types,
>> so that one should use set_pageblock_isolate() to isolate pageblocks.
>> As a result, move pageblock migratetype code out of
>> __move_freepages_block().
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>  include/linux/page-isolation.h |  5 +-
>>  mm/page_alloc.c                | 97 ++++++++++++++++++++++++++++------
>>  mm/page_isolation.c            | 21 ++++----
>>  3 files changed, 92 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
>> index 14c6a5f691c2..7241a6719618 100644
>> --- a/include/linux/page-isolation.h
>> +++ b/include/linux/page-isolation.h
>> @@ -44,10 +44,9 @@ static inline void set_pageblock_isolate(struct page *page)
>>  void __meminit init_pageblock_migratetype(struct page *page,
>>  					  enum migratetype migratetype,
>>  					  bool isolate);
>> -void set_pageblock_migratetype(struct page *page, enum migratetype 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 8fcbd7fa13c2..44a08b1a9de4 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -524,13 +524,36 @@ void clear_pfnblock_bit(const struct page *page, unsigned long pfn,
>>  	__clear_bit(bitidx + pb_bit, bitmap_word);
>>  }
>>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +/**
>> + * toggle_pfnblock_bit - Toggle a standalone bit of a pageblock
>> + * @page: The page within the block of interest
>> + * @pfn: The target page frame number
>> + * @pb_bit: pageblock bit to toggle
>> + */
>> +static void toggle_pfnblock_bit(const struct page *page, unsigned long pfn,
>> +		      enum pageblock_bits pb_bit)
>> +{
>> +	unsigned long *bitmap_word;
>> +	unsigned long bitidx;
>> +
>> +	if (WARN_ON_ONCE(pb_bit <= PB_migrate_end ||
>> +			 pb_bit >= __NR_PAGEBLOCK_BITS))
>> +		return;
>> +
>> +	get_pfnblock_bitmap_bitidx(page, pfn, &bitmap_word, &bitidx);
>> +
>> +	__change_bit(bitidx + pb_bit, bitmap_word);
>
> Again the non-atomic variant, but actually below I suggest we drop this.

Yep.

>
>> +}
>> +#endif
>> +
>>  /**
>>   * set_pageblock_migratetype - Set the migratetype of a pageblock
>>   * @page: The page within the block of interest
>>   * @migratetype: migratetype to set
>>   */
>> -__always_inline void set_pageblock_migratetype(struct page *page,
>> -					       enum migratetype migratetype)
>> +static void set_pageblock_migratetype(struct page *page,
>> +				      enum migratetype migratetype)
>>  {
>>  	unsigned long mask = MIGRATETYPE_MASK;
>>
>> @@ -540,11 +563,15 @@ __always_inline void set_pageblock_migratetype(struct page *page,
>>
>>  #ifdef CONFIG_MEMORY_ISOLATION
>>  	if (migratetype == MIGRATE_ISOLATE) {
>> -		set_pfnblock_bit(page, page_to_pfn(page), PB_migrate_isolate);
>> +		VM_WARN_ONCE(1,
>> +			"Use set_pageblock_isolate() for pageblock isolation");
>>  		return;
>>  	}
>>  	/* change mask to clear PB_migrate_isolate if it is set */
>>  	mask = MIGRATETYPE_AND_ISO_MASK;
>> +	VM_WARN_ONCE(get_pfnblock_bit(page, page_to_pfn(page),
>> +				      PB_migrate_isolate),
>> +		     "Use clear_pageblock_isolate() to unisolate pageblock");
>>  #endif
>
> We might be too paranoid with the warnings given these are all local
> functions to this file so risk of misuse should be low. Maybe we could
> remove later...

Yeah. In the next step, when struct pageblock_info is used to change
a pageblock migratetype and isolation state, these warnings should
go away, since caller will need to be explicit about isolation operations.

>
>>  	__set_pfnblock_flags_mask(page, page_to_pfn(page),
>>  				  (unsigned long)migratetype, mask);
>> @@ -1931,8 +1958,8 @@ static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
>>  #endif
>>
>>  /*
>> - * Change the type of a block and move all its free pages to that
>> - * type's freelist.
>> + * Move all free pages of a block to new type's freelist. Caller needs to
>> + * change the block type.
>>   */
>>  static int __move_freepages_block(struct zone *zone, unsigned long start_pfn,
>>  				  int old_mt, int new_mt)
>> @@ -1964,8 +1991,6 @@ static int __move_freepages_block(struct zone *zone, unsigned long start_pfn,
>>  		pages_moved += 1 << order;
>>  	}
>>
>> -	set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>> -
>>  	return pages_moved;
>>  }
>>
>> @@ -2023,11 +2048,16 @@ static int move_freepages_block(struct zone *zone, struct page *page,
>>  				int old_mt, int new_mt)
>>  {
>>  	unsigned long start_pfn;
>> +	int res;
>>
>>  	if (!prep_move_freepages_block(zone, page, &start_pfn, NULL, NULL))
>>  		return -1;
>>
>> -	return __move_freepages_block(zone, start_pfn, old_mt, new_mt);
>> +	res = __move_freepages_block(zone, start_pfn, old_mt, new_mt);
>> +	set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>> +
>> +	return res;
>> +
>>  }
>>
>>  #ifdef CONFIG_MEMORY_ISOLATION
>> @@ -2055,11 +2085,16 @@ static unsigned long find_large_buddy(unsigned long start_pfn)
>>  	return start_pfn;
>>  }
>>
>> +static inline void toggle_pageblock_isolate(struct page *page)
>> +{
>> +	toggle_pfnblock_bit(page, page_to_pfn(page), PB_migrate_isolate);
>> +}
>
> I'm wary about the togle action, as we should always know what action we
> want to do anyway. So we could just add a "bool isolate" parameter and call
> set or clear explicitly? Allows for some hypothetical DEBUG_VM checks too
> (pageblock is not already in the state we want it to be).

This function was added to follow Johannes’ suggestion of getting rid of
if statement. I can change it back, make it explicit, and add
an VM_WARN_ONCE.

Hi Johannes,

If you want the non if statement version to stay, please let me know.


Best Regards,
Yan, Zi