| Message ID | 20250214154215.717537-3-ziy@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make MIGRATE_ISOLATE a standalone bit | expand |
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? > { > 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; }
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
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) { 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); 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; } + +bool pageblock_isolate_and_move_free_pages(struct zone *zone, struct page *page) +{ + return __move_freepages_for_page_isolation(zone, page, true); +} + +bool pageblock_unisolate_and_move_free_pages(struct zone *zone, struct page *page) +{ + return __move_freepages_for_page_isolation(zone, page, false); +} + #endif /* CONFIG_MEMORY_ISOLATION */ static void change_pageblock_range(struct page *pageblock_page, diff --git a/mm/page_isolation.c b/mm/page_isolation.c index 8ed53ee00f2a..01d9a4eace7a 100644 --- a/mm/page_isolation.c +++ b/mm/page_isolation.c @@ -188,7 +188,7 @@ static int set_migratetype_isolate(struct page *page, int migratetype, int isol_ unmovable = has_unmovable_pages(check_unmovable_start, check_unmovable_end, migratetype, isol_flags); if (!unmovable) { - if (!move_freepages_block_isolate(zone, page, MIGRATE_ISOLATE)) { + if (!pageblock_isolate_and_move_free_pages(zone, page)) { spin_unlock_irqrestore(&zone->lock, flags); return -EBUSY; } @@ -209,7 +209,7 @@ static int set_migratetype_isolate(struct page *page, int migratetype, int isol_ return -EBUSY; } -static void unset_migratetype_isolate(struct page *page, int migratetype) +static void unset_migratetype_isolate(struct page *page) { struct zone *zone; unsigned long flags; @@ -262,10 +262,10 @@ static void unset_migratetype_isolate(struct page *page, int migratetype) * Isolating this block already succeeded, so this * should not fail on zone boundaries. */ - WARN_ON_ONCE(!move_freepages_block_isolate(zone, page, migratetype)); + WARN_ON_ONCE(!pageblock_unisolate_and_move_free_pages(zone, page)); } else { - set_pageblock_migratetype(page, migratetype); - __putback_isolated_page(page, order, migratetype); + clear_pageblock_isolate(page); + __putback_isolated_page(page, order, get_pageblock_migratetype(page)); } zone->nr_isolate_pageblock--; out: @@ -383,7 +383,7 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags, if (PageBuddy(page)) { int order = buddy_order(page); - /* move_freepages_block_isolate() handled this */ + /* pageblock_isolate_and_move_free_pages() handled this */ VM_WARN_ON_ONCE(pfn + (1 << order) > boundary_pfn); pfn += 1UL << order; @@ -433,7 +433,7 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags, failed: /* restore the original migratetype */ if (!skip_isolation) - unset_migratetype_isolate(pfn_to_page(isolate_pageblock), migratetype); + unset_migratetype_isolate(pfn_to_page(isolate_pageblock)); return -EBUSY; } @@ -504,7 +504,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, ret = isolate_single_pageblock(isolate_end, flags, true, skip_isolation, migratetype); if (ret) { - unset_migratetype_isolate(pfn_to_page(isolate_start), migratetype); + unset_migratetype_isolate(pfn_to_page(isolate_start)); return ret; } @@ -517,8 +517,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, start_pfn, end_pfn)) { undo_isolate_page_range(isolate_start, pfn, migratetype); unset_migratetype_isolate( - pfn_to_page(isolate_end - pageblock_nr_pages), - migratetype); + pfn_to_page(isolate_end - pageblock_nr_pages)); return -EBUSY; } } @@ -548,7 +547,7 @@ void undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, page = __first_valid_page(pfn, pageblock_nr_pages); if (!page || !is_migrate_isolate_page(page)) continue; - unset_migratetype_isolate(page, migratetype); + unset_migratetype_isolate(page); } } /*
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(-)