[PATCH] mm/compaction: Fix bug in hugetlb handling pathway

[PATCH] mm/compaction: Fix bug in hugetlb handling pathway

[Vishal Moola (Oracle)]

The compaction code doesn't take references on pages until we're certain
we should attempt to handle it.

In the hugetlb case, isolate_or_dissolve_huge_page() may return -EBUSY
without taking a reference to the folio associated with our pfn. If our
folio's refcount drops to 0, compound_nr() becomes unpredictable, making
low_pfn and nr_scanned unreliable.
The user-visible effect is minimal - this should rarely happen (if ever).

Fix this by storing the folio statistics earlier on the stack (just like
the THP and Buddy cases).

Also revert commit 66fe1cf7f581 ("mm: compaction: use helper compound_nr
in isolate_migratepages_block")
to make backporting easier.

Fixes: 369fa227c219 ("mm: make alloc_contig_range handle free hugetlb pages")
Cc: Miaohe Lin <linmiaohe@huawei.com>
Cc: Oscar Salvador <osalvador@suse.de>
Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 mm/compaction.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 139f00c0308a..ca71fd3c3181 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -981,13 +981,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 		}
 
 		if (PageHuge(page)) {
+			const unsigned int order = compound_order(page);
 			/*
 			 * skip hugetlbfs if we are not compacting for pages
 			 * bigger than its order. THPs and other compound pages
 			 * are handled below.
 			 */
 			if (!cc->alloc_contig) {
-				const unsigned int order = compound_order(page);
 
 				if (order <= MAX_PAGE_ORDER) {
 					low_pfn += (1UL << order) - 1;
@@ -1011,8 +1011,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				 /* Do not report -EBUSY down the chain */
 				if (ret == -EBUSY)
 					ret = 0;
-				low_pfn += compound_nr(page) - 1;
-				nr_scanned += compound_nr(page) - 1;
+				low_pfn += (1UL << order) - 1;
+				nr_scanned += (1UL << order) - 1;
 				goto isolate_fail;
 			}
 
-- 
2.48.1

[PATCH] mm/compaction: Use folio in hugetlb pathway

[Vishal Moola (Oracle)]

Use a folio in the hugetlb pathway during the compaction migrate-able
pageblock scan.

This removes a call to compound_head().

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 include/linux/hugetlb.h | 4 ++--
 mm/compaction.c         | 8 ++++----
 mm/hugetlb.c            | 3 +--
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 8f3ac832ee7f..a57bed83c657 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -695,7 +695,7 @@ struct huge_bootmem_page {
 
 bool hugetlb_bootmem_page_zones_valid(int nid, struct huge_bootmem_page *m);
 
-int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list);
+int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list);
 int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn);
 void wait_for_freed_hugetlb_folios(void);
 struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
@@ -1083,7 +1083,7 @@ static inline struct folio *filemap_lock_hugetlb_folio(struct hstate *h,
 	return NULL;
 }
 
-static inline int isolate_or_dissolve_huge_page(struct page *page,
+static inline int isolate_or_dissolve_huge_folio(struct folio *folio,
 						struct list_head *list)
 {
 	return -ENOMEM;
diff --git a/mm/compaction.c b/mm/compaction.c
index ca71fd3c3181..dd868c861774 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1001,10 +1001,11 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				locked = NULL;
 			}
 
-			ret = isolate_or_dissolve_huge_page(page, &cc->migratepages);
+			folio = page_folio(page);
+			ret = isolate_or_dissolve_huge_folio(folio, &cc->migratepages);
 
 			/*
-			 * Fail isolation in case isolate_or_dissolve_huge_page()
+			 * Fail isolation in case isolate_or_dissolve_huge_folio()
 			 * reports an error. In case of -ENOMEM, abort right away.
 			 */
 			if (ret < 0) {
@@ -1016,12 +1017,11 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				goto isolate_fail;
 			}
 
-			if (PageHuge(page)) {
+			if (folio_test_hugetlb(folio)) {
 				/*
 				 * Hugepage was successfully isolated and placed
 				 * on the cc->migratepages list.
 				 */
-				folio = page_folio(page);
 				low_pfn += folio_nr_pages(folio) - 1;
 				goto isolate_success_no_list;
 			}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 39f92aad7bd1..93bc8c4c904b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2896,10 +2896,9 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
 	return ret;
 }
 
-int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list)
+int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list)
 {
 	struct hstate *h;
-	struct folio *folio = page_folio(page);
 	int ret = -EBUSY;
 
 	/*
-- 
2.48.1

Re: [PATCH] mm/compaction: Fix bug in hugetlb handling pathway

[Oscar Salvador]

On Mon, Mar 31, 2025 at 07:10:24PM -0700, Vishal Moola (Oracle) wrote:
> The compaction code doesn't take references on pages until we're certain
> we should attempt to handle it.
> 
> In the hugetlb case, isolate_or_dissolve_huge_page() may return -EBUSY
> without taking a reference to the folio associated with our pfn. If our
> folio's refcount drops to 0, compound_nr() becomes unpredictable, making
> low_pfn and nr_scanned unreliable.
> The user-visible effect is minimal - this should rarely happen (if ever).

So, with compound_order() we either return the real order of the
compound page or '0', right?

> Fix this by storing the folio statistics earlier on the stack (just like
> the THP and Buddy cases).
> 
> Also revert commit 66fe1cf7f581 ("mm: compaction: use helper compound_nr
> in isolate_migratepages_block")
> to make backporting easier.
> 
> Fixes: 369fa227c219 ("mm: make alloc_contig_range handle free hugetlb pages")
> Cc: Miaohe Lin <linmiaohe@huawei.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>

Acked-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm/compaction: Use folio in hugetlb pathway

[Oscar Salvador]

On Mon, Mar 31, 2025 at 07:10:25PM -0700, Vishal Moola (Oracle) wrote:
> Use a folio in the hugetlb pathway during the compaction migrate-able
> pageblock scan.
> 
> This removes a call to compound_head().
> 
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>

Acked-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm/compaction: Fix bug in hugetlb handling pathway

[Zi Yan]

On 31 Mar 2025, at 22:10, Vishal Moola (Oracle) wrote:

> The compaction code doesn't take references on pages until we're certain
> we should attempt to handle it.
>
> In the hugetlb case, isolate_or_dissolve_huge_page() may return -EBUSY
> without taking a reference to the folio associated with our pfn. If our
> folio's refcount drops to 0, compound_nr() becomes unpredictable, making
> low_pfn and nr_scanned unreliable.
> The user-visible effect is minimal - this should rarely happen (if ever).
>
> Fix this by storing the folio statistics earlier on the stack (just like
> the THP and Buddy cases).
>
> Also revert commit 66fe1cf7f581 ("mm: compaction: use helper compound_nr
> in isolate_migratepages_block")
> to make backporting easier.
>
> Fixes: 369fa227c219 ("mm: make alloc_contig_range handle free hugetlb pages")
> Cc: Miaohe Lin <linmiaohe@huawei.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> ---
>  mm/compaction.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>

<snip>

> @@ -1011,8 +1011,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  				 /* Do not report -EBUSY down the chain */
>  				if (ret == -EBUSY)
>  					ret = 0;
> -				low_pfn += compound_nr(page) - 1;
> -				nr_scanned += compound_nr(page) - 1;
> +				low_pfn += (1UL << order) - 1;
> +				nr_scanned += (1UL << order) - 1;
>  				goto isolate_fail;
>  			}
>

Right after this, there is “low_pfn += folio_nr_pages(folio) - 1” for
isolated hugetlb. I wonder if that can use order as well. Maybe not,
since the order is obtained without taking a reference, but folio_nr_pages()
is called with a reference. They might be different.

Anyway, Reviewed-by: Zi Yan <ziy@nvidia.com>


Best Regards,
Yan, Zi

Re: [PATCH] mm/compaction: Use folio in hugetlb pathway

[Zi Yan]

On 31 Mar 2025, at 22:10, Vishal Moola (Oracle) wrote:

> Use a folio in the hugetlb pathway during the compaction migrate-able
> pageblock scan.
>
> This removes a call to compound_head().
>
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> ---
>  include/linux/hugetlb.h | 4 ++--
>  mm/compaction.c         | 8 ++++----
>  mm/hugetlb.c            | 3 +--
>  3 files changed, 7 insertions(+), 8 deletions(-)
>
LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com>

Best Regards,
Yan, Zi

Re: [PATCH] mm/compaction: Fix bug in hugetlb handling pathway

[Vishal Moola (Oracle)]

On Tue, Apr 01, 2025 at 04:59:46PM +0200, Oscar Salvador wrote:
> On Mon, Mar 31, 2025 at 07:10:24PM -0700, Vishal Moola (Oracle) wrote:
> > The compaction code doesn't take references on pages until we're certain
> > we should attempt to handle it.
> > 
> > In the hugetlb case, isolate_or_dissolve_huge_page() may return -EBUSY
> > without taking a reference to the folio associated with our pfn. If our
> > folio's refcount drops to 0, compound_nr() becomes unpredictable, making
> > low_pfn and nr_scanned unreliable.
> > The user-visible effect is minimal - this should rarely happen (if ever).
> 
> So, with compound_order() we either return the real order of the
> compound page or '0', right?

Yup. There's a world in which that folio could be freed and reallocated
as part of another large order page as well (where it would return the
order of that folio).

> > Fix this by storing the folio statistics earlier on the stack (just like
> > the THP and Buddy cases).
> > 
> > Also revert commit 66fe1cf7f581 ("mm: compaction: use helper compound_nr
> > in isolate_migratepages_block")
> > to make backporting easier.
> > 
> > Fixes: 369fa227c219 ("mm: make alloc_contig_range handle free hugetlb pages")
> > Cc: Miaohe Lin <linmiaohe@huawei.com>
> > Cc: Oscar Salvador <osalvador@suse.de>
> > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> 
> Acked-by: Oscar Salvador <osalvador@suse.de>
> 
> 
> -- 
> Oscar Salvador
> SUSE Labs

Re: [PATCH] mm/compaction: Fix bug in hugetlb handling pathway

[Vishal Moola (Oracle)]

On Tue, Apr 01, 2025 at 11:20:48AM -0400, Zi Yan wrote:
> On 31 Mar 2025, at 22:10, Vishal Moola (Oracle) wrote:
> 
> > The compaction code doesn't take references on pages until we're certain
> > we should attempt to handle it.
> >
> > In the hugetlb case, isolate_or_dissolve_huge_page() may return -EBUSY
> > without taking a reference to the folio associated with our pfn. If our
> > folio's refcount drops to 0, compound_nr() becomes unpredictable, making
> > low_pfn and nr_scanned unreliable.
> > The user-visible effect is minimal - this should rarely happen (if ever).
> >
> > Fix this by storing the folio statistics earlier on the stack (just like
> > the THP and Buddy cases).
> >
> > Also revert commit 66fe1cf7f581 ("mm: compaction: use helper compound_nr
> > in isolate_migratepages_block")
> > to make backporting easier.
> >
> > Fixes: 369fa227c219 ("mm: make alloc_contig_range handle free hugetlb pages")
> > Cc: Miaohe Lin <linmiaohe@huawei.com>
> > Cc: Oscar Salvador <osalvador@suse.de>
> > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> > ---
> >  mm/compaction.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> 
> <snip>
> 
> > @@ -1011,8 +1011,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >  				 /* Do not report -EBUSY down the chain */
> >  				if (ret == -EBUSY)
> >  					ret = 0;
> > -				low_pfn += compound_nr(page) - 1;
> > -				nr_scanned += compound_nr(page) - 1;
> > +				low_pfn += (1UL << order) - 1;
> > +				nr_scanned += (1UL << order) - 1;
> >  				goto isolate_fail;
> >  			}
> >
> 
> Right after this, there is “low_pfn += folio_nr_pages(folio) - 1” for
> isolated hugetlb. I wonder if that can use order as well. Maybe not,
> since the order is obtained without taking a reference, but folio_nr_pages()
> is called with a reference. They might be different.

I thought about that as well. There's a very unlikely case where they
could be different: We had a hugetlb page, free'd it, then allocated it
to another hugetlb page. At this point (the success path) we would want
the rest of the compaction counters all the sync up to whichever folio
we ended up isolating anyways.

> Anyway, Reviewed-by: Zi Yan <ziy@nvidia.com>

Thanks!

> 
> Best Regards,
> Yan, Zi