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

[Wei Yang <richard.weiyang@gmail.com>]

On Fri, Sep 12, 2025 at 10:22:01AM -0700, Vishal Moola (Oracle) wrote:
[...]
>> >>>>>>> ---
>> >>>>>>> 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.

Thanks :-)

-- 
Wei Yang
Help you, Help me