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?
> {
> 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));
> 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;
}
