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

Vishal Moola (Oracle)
2025-04-01T18:09:38Z

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