[RFC 0/1] Convert is_migrate_isolate_page() to is_migrate_isolate_folio()

[RFC 0/1] Convert is_migrate_isolate_page() to is_migrate_isolate_folio()

[nifan.cxl]

From: Fan Ni <fan.ni@samsung.com>

Sending out this patch per Matthew Wilcox's suggestion 
that we need to convert is_migrate_isolate_page() to use folio
https://lore.kernel.org/linux-mm/Z_XmUrbxKtYmzmJ6@casper.infradead.org/

However, when looking into the code, I have noticed that among the uers
of is_migrate_isolate_page(), in most cases the page passed in is from a 
a pageblock. 
I am not sure how we should proceed with these cases.
Should we deal with pageblock or just leave it as it is and only do the page
to folio conversion for the pages within?

So This RFC is mainly sent for collecting input about how to move forward.



Fan Ni (1):
  mm: Convert is_migrate_isolate_page() to is_migrate_isolate_folio()

 include/linux/page-isolation.h |  6 +++---
 mm/hugetlb.c                   |  2 +-
 mm/page_isolation.c            | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.47.2

[RFC 1/1] mm: Convert is_migrate_isolate_page() to is_migrate_isolate_folio()

[nifan.cxl]

From: Fan Ni <fan.ni@samsung.com>

Convert is_migrate_isolate_page() to is_migrate_isolate_folio() to take
folio directly.

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 include/linux/page-isolation.h |  6 +++---
 mm/hugetlb.c                   |  2 +-
 mm/page_isolation.c            | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
index 898bb788243b..74d6a8cf4960 100644
--- a/include/linux/page-isolation.h
+++ b/include/linux/page-isolation.h
@@ -3,16 +3,16 @@
 #define __LINUX_PAGEISOLATION_H
 
 #ifdef CONFIG_MEMORY_ISOLATION
-static inline bool is_migrate_isolate_page(struct page *page)
+static inline bool is_migrate_isolate_folio(struct folio *folio)
 {
-	return get_pageblock_migratetype(page) == MIGRATE_ISOLATE;
+	return folio_migratetype(folio) == MIGRATE_ISOLATE;
 }
 static inline bool is_migrate_isolate(int migratetype)
 {
 	return migratetype == MIGRATE_ISOLATE;
 }
 #else
-static inline bool is_migrate_isolate_page(struct page *page)
+static inline bool is_migrate_isolate_folio(struct folio *folio)
 {
 	return false;
 }
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 0057d1f1dc9a..1e712dc4783a 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1306,7 +1306,7 @@ static struct folio *dequeue_hugetlb_folio_node_exact(struct hstate *h,
 		if (folio_test_hwpoison(folio))
 			continue;
 
-		if (is_migrate_isolate_page(&folio->page))
+		if (is_migrate_isolate_folio(folio))
 			continue;
 
 		list_move(&folio->lru, &h->hugepage_activelist);
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index b2fc5266e3d2..540d2add4834 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -169,7 +169,7 @@ static int set_migratetype_isolate(struct page *page, int migratetype, int isol_
 	 * If it is already set, then someone else must have raced and
 	 * set it before us.
 	 */
-	if (is_migrate_isolate_page(page)) {
+	if (is_migrate_isolate_folio(page_folio(page))) {
 		spin_unlock_irqrestore(&zone->lock, flags);
 		return -EBUSY;
 	}
@@ -219,7 +219,7 @@ static void unset_migratetype_isolate(struct page *page, int migratetype)
 
 	zone = page_zone(page);
 	spin_lock_irqsave(&zone->lock, flags);
-	if (!is_migrate_isolate_page(page))
+	if (!is_migrate_isolate_folio(page_folio(page)))
 		goto out;
 
 	/*
@@ -235,7 +235,7 @@ static void unset_migratetype_isolate(struct page *page, int migratetype)
 		if (order >= pageblock_order && order < MAX_PAGE_ORDER) {
 			buddy = find_buddy_page_pfn(page, page_to_pfn(page),
 						    order, NULL);
-			if (buddy && !is_migrate_isolate_page(buddy)) {
+			if (buddy && !is_migrate_isolate_folio(page_folio(buddy))) {
 				isolated_page = !!__isolate_free_page(page, order);
 				/*
 				 * Isolating a free page in an isolated pageblock
@@ -546,7 +546,7 @@ void undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
 	     pfn < isolate_end;
 	     pfn += pageblock_nr_pages) {
 		page = __first_valid_page(pfn, pageblock_nr_pages);
-		if (!page || !is_migrate_isolate_page(page))
+		if (!page || !is_migrate_isolate_folio(page_folio(page)))
 			continue;
 		unset_migratetype_isolate(page, migratetype);
 	}
@@ -631,7 +631,7 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn,
 	 */
 	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
 		page = __first_valid_page(pfn, pageblock_nr_pages);
-		if (page && !is_migrate_isolate_page(page))
+		if (page && !is_migrate_isolate_folio(page_folio(page)))
 			break;
 	}
 	page = __first_valid_page(start_pfn, end_pfn - start_pfn);
-- 
2.47.2

Re: [RFC 0/1] Convert is_migrate_isolate_page() to is_migrate_isolate_folio()

[Matthew Wilcox]

On Tue, May 06, 2025 at 11:38:28AM -0700, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
> 
> Sending out this patch per Matthew Wilcox's suggestion 
> that we need to convert is_migrate_isolate_page() to use folio
> https://lore.kernel.org/linux-mm/Z_XmUrbxKtYmzmJ6@casper.infradead.org/

That's not what I said!

This is what I said:
> >  
> > -		if (is_migrate_isolate_page(&folio->page))
> > +		if (is_migrate_isolate_page(folio_page(folio, 0)))
> >  			continue;
>
> I think we need an is_migrate_isolate_folio() instead of this.

> However, when looking into the code, I have noticed that among the uers
> of is_migrate_isolate_page(), in most cases the page passed in is from a 
> a pageblock. 
> I am not sure how we should proceed with these cases.
> Should we deal with pageblock or just leave it as it is and only do the page
> to folio conversion for the pages within?

Neither.  Add a folio_test_migrate_isolate() in addition to
is_migrate_isolate_page().  Don't force a conversion as it's a
legitimate question to ask of pages as well as of folios.
And some of the pages you want to ask it of may well not be part of
folios (they may be part of a slab or some other memdesc).

Re: [RFC 0/1] Convert is_migrate_isolate_page() to is_migrate_isolate_folio()

[Fan Ni]

On Tue, May 06, 2025 at 08:08:55PM +0100, Matthew Wilcox wrote:
> On Tue, May 06, 2025 at 11:38:28AM -0700, nifan.cxl@gmail.com wrote:
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > Sending out this patch per Matthew Wilcox's suggestion 
> > that we need to convert is_migrate_isolate_page() to use folio
> > https://lore.kernel.org/linux-mm/Z_XmUrbxKtYmzmJ6@casper.infradead.org/
> 
> That's not what I said!
> 
> This is what I said:
> > >  
> > > -		if (is_migrate_isolate_page(&folio->page))
> > > +		if (is_migrate_isolate_page(folio_page(folio, 0)))
> > >  			continue;
> >
> > I think we need an is_migrate_isolate_folio() instead of this.
> 
> > However, when looking into the code, I have noticed that among the uers
> > of is_migrate_isolate_page(), in most cases the page passed in is from a 
> > a pageblock. 
> > I am not sure how we should proceed with these cases.
> > Should we deal with pageblock or just leave it as it is and only do the page
> > to folio conversion for the pages within?
> 
> Neither.  Add a folio_test_migrate_isolate() in addition to
> is_migrate_isolate_page().  Don't force a conversion as it's a
> legitimate question to ask of pages as well as of folios.
> And some of the pages you want to ask it of may well not be part of
> folios (they may be part of a slab or some other memdesc).

Oh. I misunderstood "we need ... instead of .." :-(.
Thanks for the the clarification.

Another separate question.

We have a free_frozen_pages(page, order), which have two types of users
1) head page and order directly from a struct folio; or
2) page pointer that does not neccesarily be the head page of a
folio and order that may not be directly related to a folio;

Does it make sense to introduce a dedicate function like
free_frozen_folio(struct folio *folio) to handle case 1)?

Fan



-- 
Fan Ni

Re: [RFC 0/1] Convert is_migrate_isolate_page() to is_migrate_isolate_folio()

[Matthew Wilcox]

On Tue, May 06, 2025 at 12:23:15PM -0700, Fan Ni wrote:
> We have a free_frozen_pages(page, order), which have two types of users
> 1) head page and order directly from a struct folio; or
> 2) page pointer that does not neccesarily be the head page of a
> folio and order that may not be directly related to a folio;
> 
> Does it make sense to introduce a dedicate function like
> free_frozen_folio(struct folio *folio) to handle case 1)?

No.  free_frozen_pages() will eventually be just free_pages()
when struct page has lost its refcount (as the refcount will have moved
into the memdescs which need it).  It's premature to do anything to it
now, we have many steps to go.