Re: [PATCH] mm/compaction: fix low_pfn advance on isolating hugetlb

["Vishal Moola (Oracle)" <vishal.moola@gmail.com>]

On Thu, Sep 11, 2025 at 09:29:39PM -0400, Zi Yan wrote:
> On 11 Sep 2025, at 21:07, Wei Yang wrote:
> 
> > On Thu, Sep 11, 2025 at 10:27:08AM -0700, Vishal Moola (Oracle) wrote:
> >> On Thu, Sep 11, 2025 at 12:19:34PM -0400, Zi Yan wrote:
> >>> On 10 Sep 2025, at 23:27, Wei Yang wrote:
> >>>
> >>>> On Wed, Sep 10, 2025 at 09:35:53PM -0400, Zi Yan wrote:
> >>>>> On 10 Sep 2025, at 21:25, Wei Yang wrote:
> >>>>>
> >>>>>> On Wed, Sep 10, 2025 at 09:22:40AM +0000, Wei Yang wrote:
> >>>>>>> Commit 56ae0bb349b4 ("mm: compaction: convert to use a folio in
> >>>>>>> isolate_migratepages_block()") converts api from page to folio. But the
> >>>>>>> low_pfn advance for hugetlb page seems wrong when low_pfn doesn't point
> >>>>>>> to head page.
> >>>>>>>
> >>>>>>> Originally, if page is a hugetlb tail page, compound_nr() return 1,
> >>>>>>> which means low_pfn only advance one in next iteration. After the
> >>>>>>> change, low_pfn would advance more than the hugetlb range, since
> >>>>>>> folio_nr_pages() always return total number of the large page. This
> >>>>>>> results in skipping some range to isolate and then to migrate.
> >>>>>>>
> >>>>>>> The worst case for alloc_contig is it does all the isolation and
> >>>>>>> migration, but finally find some range is still not isolated. And then
> >>>>>>> undo all the work and try a new range.
> >>>>>>>
> >>>>>>> Advance low_pfn to the end of hugetlb.
> >>>>>>>
> >>>>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >>>>>>> Fixes: 56ae0bb349b4 ("mm: compaction: convert to use a folio in isolate_migratepages_block()")
> >>>>>>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> >>>>>>> Cc: Oscar Salvador <osalvador@suse.de>
> >>>>>>
> >>>>>> Forgot to cc stable.
> >>>>>>
> >>>>>> Cc: <stable@vger.kernel.org>
> >>>>>
> >>>>> Is there any bug report to justify the backport? Since it is more likely
> >>>>> to be a performance issue instead of a correctness issue.
> >>>>>
> >>>>
> >>>> OK, I thought cc-stable is paired with fixes tag.
> >>>>
> >>>> If not, please drop it.
> >>>>
> >>>>>>
> >>>>>>> ---
> >>>>>>> mm/compaction.c | 2 +-
> >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/mm/compaction.c b/mm/compaction.c
> >>>>>>> index bf021b31c7ec..1e8f8eca318c 100644
> >>>>>>> --- a/mm/compaction.c
> >>>>>>> +++ b/mm/compaction.c
> >>>>>>> @@ -989,7 +989,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >>>>>>> 				 * Hugepage was successfully isolated and placed
> >>>>>>> 				 * on the cc->migratepages list.
> >>>>>>> 				 */
> >>>>>>> -				low_pfn += folio_nr_pages(folio) - 1;
> >>>>>>> +				low_pfn += folio_nr_pages(folio) - folio_page_idx(folio, page) - 1;
> >>>>>>
> >>>>>> One question is why we advance compound_nr() in original version.
> >>>>>>
> >>>>>> Yes, there are several places advancing compound_nr(), but it seems to iterate
> >>>>>> on the same large page and do the same thing and advance 1 again.
> >>>>>>
> >>>>>> Not sure which part story I missed.
> >>>>>
> >>>>> isolate_migratepages_block() starts from the beginning of a pageblock.
> >>>>> How likely the code hit in the middle of a hugetlb?
> >>>>>
> >>>>
> >>>> OK, this is a kind of optimization based on the knowledge it is not likely to
> >>>> be a tail page?
> >>>
> >>> No, it might be that most of the time page is the head, or people assume so.
> >>
> >> For compound pages, we will always have tail pfn < head pfn, so we should
> >> always find the head page first.
> >>
> >
> > I think you want to say tail pfn > head pfn?
> >
> >> If you did find a case where we somehow encounter a tail page here, I'd
> >> love to see it. And then you'd also want to make sure the other compaction
> >> trackers are appropriately accounted for.
> >
> > I may not follow you here, below is the call flow for
> > isolate_migratepages_block() invoked during __alloc_contig_pages().
> >
> > __alloc_contig_pages(nr_pages, ..);
> >     start = ALIGN(zone->zone_start_pfn, nr_pages);
> >     alloc_contig_range_noprof(start, ..);
> >         __alloc_contig_migrate_range(.., start, ..);
> >             pfn = start;
> >             isolate_migratepages_range(.., pfn, ..);
> >                 isolate_migratepages_block(.., pfn, ..);
> >                     page = pfn_to_page(pfn);
> >     start += nr_pages;
> >
> > In the loop of __alloc_contig_pages(), it iterate on each nr_pages range. And
> > nr_pages seems could be any positive number, so it looks the first pfn checked
> > by isolate_migratepages_block() could be not aligned with page order or less
> > than MAX_PAGE_ORDER. This mean it could be a tail page per my understanding.
> >
> > Maybe I missed some point here?

That looks right, I missed the comment:
        /* Scan block by block. First and last block may be incomplete */

> You are right.
> 
> But nr_pages cannot be any positive number, since ALIGN only accepts
> power of 2 as the alignment. So alloc_contig_pages() might need another
> fix to handle the case nr_pages is not power of 2.
> 
> Oh, after I checked pfn_range_valid_contig(), I find your example does not
> apply, since it returns false when any page in the range is PageHuge.
> This means with your example, PageHuge branch will never be executed.
> But alloc_contig_range_noprof() is exported and can be used directly,
> the @start input can be any pfn, which can be in the middle of PageHuge.
> So your fix is still needed for this case.

Makes sense to me.