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@xxxxxxxxxx>
>> ---
>> 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
