Re: [PATCH V2 0/6] mm: page_alloc: freelist migratetype hygiene

[Zi Yan]

On 11 Oct 2023, at 11:45, Johannes Weiner wrote:

> On Wed, Oct 11, 2023 at 11:25:27AM -0400, Johannes Weiner wrote:
>> On Tue, Oct 10, 2023 at 05:12:01PM -0400, Johannes Weiner wrote:
>>> On Mon, Oct 02, 2023 at 10:26:44PM -0400, Zi Yan wrote:
>>>> @@ -1614,10 +1652,43 @@ static int move_freepages(struct zone *zone, unsigned long start_pfn,
>>>>
>>>>  		order = buddy_order(page);
>>>>  		move_to_free_list(page, zone, order, old_mt, new_mt);
>>>> +		/*
>>>> +		 * set page migratetype 1) only after we move all free pages in
>>>> +		 * one pageblock and 2) for all pageblocks within the page.
>>>> +		 *
>>>> +		 * for 1), since move_to_free_list() checks page migratetype with
>>>> +		 * old_mt and changing one page migratetype affects all pages
>>>> +		 * within the same pageblock, if we are moving more than
>>>> +		 * one free pages in the same pageblock, setting migratetype
>>>> +		 * right after first move_to_free_list() triggers the warning
>>>> +		 * in the following move_to_free_list().
>>>> +		 *
>>>> +		 * for 2), when a free page order is greater than pageblock_order,
>>>> +		 * all pageblocks within the free page need to be changed after
>>>> +		 * move_to_free_list().
>>>
>>> I think this can be somewhat simplified.
>>>
>>> There are two assumptions we can make. Buddies always consist of 2^n
>>> pages. And buddies and pageblocks are naturally aligned. This means
>>> that if this pageblock has the start of a buddy that straddles into
>>> the next pageblock(s), it must be the first page in the block. That in
>>> turn means we can move the handling before the loop.
>>
>> Eh, scratch that. Obviously, a sub-block buddy can straddle blocks :(
>
> I apologize for the back and forth, but I think I had it right the
> first time. Say we have order-0 frees at pfn 511 and 512. Those can't
> merge because their order-0 buddies are 510 and 513 respectively. The
> same keeps higher-order merges below block size within the pageblock.
> So again, due to the pow2 alignment, the only way for a buddy to
> straddle a pageblock boundary is if it's >pageblock_order.
>
> Please double check me on this, because I've stared at your patches
> and the allocator code long enough now to thoroughly confuse myself.
>
> My proposal for the follow-up changes still stands for now.

Sure. I admit that current alloc_contig_range() code is too complicated
and I am going to refactor it.

find_straddling_buddy() looks good to me. You will this change in
alloc_contig_range() to replace get_freepage_start_pfn():

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a86025f5e80a..e8ed25c94863 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6209,7 +6209,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
                       unsigned migratetype, gfp_t gfp_mask)
 {
        unsigned long outer_start, outer_end;
-       int order;
        int ret = 0;

        struct compact_control cc = {
@@ -6283,21 +6282,13 @@ int alloc_contig_range(unsigned long start, unsigned long end,
         * isolated thus they won't get removed from buddy.
         */

-       order = 0;
-       outer_start = get_freepage_start_pfn(start);
-
-       if (outer_start != start) {
-               order = buddy_order(pfn_to_page(outer_start));
-
-               /*
-                * outer_start page could be small order buddy page and
-                * it doesn't include start page. Adjust outer_start
-                * in this case to report failed page properly
-                * on tracepoint in test_pages_isolated()
-                */
-               if (outer_start + (1UL << order) <= start)
-                       outer_start = start;
-       }
+       /*
+        * outer_start page could be small order buddy page and it doesn't
+        * include start page. outer_start is set to start in
+        * find_straddling_buddy() to report failed page properly on tracepoint
+        * in test_pages_isolated()
+        */
+       outer_start = find_straddling_buddy(start);

        /* Make sure the range is really isolated. */
        if (test_pages_isolated(outer_start, end, 0)) {

Let me go through your move_freepages() in details and get back to you.

Thank you for the feedback!

--
Best Regards,
Yan, Zi

Attachment: signature.asc
Description: OpenPGP digital signature