Re: [PATCH] mm/page_alloc: find_large_buddy() from start_pfn aligned order

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

On Sat, Aug 30, 2025 at 01:25:05AM +0000, Wei Yang wrote:
> On Thu, Aug 28, 2025 at 11:02:33PM -0400, Zi Yan wrote:
> >On 28 Aug 2025, at 5:16, Wei Yang wrote:
> >
> >> We iterate pfn from order 0 to MAX_PAGE_ORDER aligned to find large
> >> buddy. While if the order is less than start_pfn aligned order, we would
> >> get the same pfn and do the same check again.
> >>
> >> Iterate from start_pfn aligned order to reduce duplicated work.
> >>
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >> Cc: Johannes Weiner <hannes@cmpxchg.org>
> >> Cc: Zi Yan <ziy@nvidia.com>
> >> Cc: Vlastimil Babka <vbabka@suse.cz>
> >> Cc: David Hildenbrand <david@redhat.com>
> >>
> >> ---
> >I think it is right, but the code is very subtle and hard to understand
> >after the change. It is better to add comment to explain it.
> 
> One thing I want to point out is in __move_freepages_block_isolate(),
> find_large_buddy() is always given a pageblock aligned start_pfn. This means
> if start_pfn is not a free page, it would always try 10 times until give up.
> 
> >
> >Paste the code below for more context:
> >
> >        while (!PageBuddy(page = pfn_to_page(pfn))) {
> >                /* Nothing found */
> >                if (++order > MAX_PAGE_ORDER)
> >                        return start_pfn;
> >                pfn &= ~0UL << order;
> >        }
> >
> >
> >
> >The code tries to find a PageBuddy starting from start_pfn starting from
> >order=0. When entering the while loop, it means PageBuddy cannot be order-0
> >and ++order increases the order by 1. Your change fast forwards the process
> >based on start_pfn. If start_pfn is not an order-0 page, based on first
> >set bit in start_pfn and how buddy page is chosen, the next possible PageBuddy
> >order can only be __ffs(start_pfn) + 1. Your code starts order at __ffs(start_pfn)
> >and it works because "if (++order > MAX_PAGE_ORDER)" increases order
> >to __ffs(start_pfn) + 1.
> >
> >Can you add a comment on your "int order = ..."? Something like:
> >
> >If start_pfn is not an order-0 PageBuddy, next PageBuddy containing start_pfn
> >has minimal order of __ffs(start_pfn) + 1. Fastforward order to __ffs(start_pfn)
> >to remove unnecessary work in the while below.
> 
> Sure, I would add a comment above the assignment.
> 
> But in my mind, we are not farst forward order, but start check from order of
> __ffs(start_pfn).
> 
> 
> How about: (not good at commento)
> 
> 	/*
> 	 * We start find large buddy from start_pfn order, since a
> 	 * !PageBuddy() means all lower order page is !PageBuddy().
> 	 */

Personally, the way Zi worded it makes more sense to me. Maybe replacing
the second sentence with your comment is fine, but I think Zi's first
sentence provides important context in a clear way.