LinuxLists.cc - Re: [PATCH V2 0/6] mm: page_alloc: freelist migratetype hygiene

2023-09-21 22:57:55

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

On 21 Sep 2023, at 6:19, David Hildenbrand wrote:

> On 21.09.23 04:31, Zi Yan wrote:
>> On 20 Sep 2023, at 13:23, Zi Yan wrote:
>>
>>> On 20 Sep 2023, at 12:04, Johannes Weiner wrote:
>>>
>>>> On Wed, Sep 20, 2023 at 09:48:12AM -0400, Johannes Weiner wrote:
>>>>> On Wed, Sep 20, 2023 at 08:07:53AM +0200, Vlastimil Babka wrote:
>>>>>> On 9/20/23 03:38, Zi Yan wrote:
>>>>>>> On 19 Sep 2023, at 20:32, Mike Kravetz wrote:
>>>>>>>
>>>>>>>> On 09/19/23 16:57, Zi Yan wrote:
>>>>>>>>> On 19 Sep 2023, at 14:47, Mike Kravetz wrote:
>>>>>>>>>
>>>>>>>>>> --- a/mm/page_alloc.c
>>>>>>>>>> +++ b/mm/page_alloc.c
>>>>>>>>>> @@ -1651,8 +1651,13 @@ static bool prep_move_freepages_block(struct zone *zone, struct page *page,
>>>>>>>>>> end = pageblock_end_pfn(pfn) - 1;
>>>>>>>>>>
>>>>>>>>>> /* Do not cross zone boundaries */
>>>>>>>>>> +#if 0
>>>>>>>>>> if (!zone_spans_pfn(zone, start))
>>>>>>>>>> start = zone->zone_start_pfn;
>>>>>>>>>> +#else
>>>>>>>>>> + if (!zone_spans_pfn(zone, start))
>>>>>>>>>> + start = pfn;
>>>>>>>>>> +#endif
>>>>>>>>>> if (!zone_spans_pfn(zone, end))
>>>>>>>>>> return false;
>>>>>>>>>> I can still trigger warnings.
>>>>>>>>>
>>>>>>>>> OK. One thing to note is that the page type in the warning changed from
>>>>>>>>> 5 (MIGRATE_ISOLATE) to 0 (MIGRATE_UNMOVABLE) with my suggested change.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Just to be really clear,
>>>>>>>> - the 5 (MIGRATE_ISOLATE) warning was from the __alloc_pages call path.
>>>>>>>> - the 0 (MIGRATE_UNMOVABLE) as above was from the alloc_contig_range call
>>>>>>>> path WITHOUT your change.
>>>>>>>>
>>>>>>>> I am guessing the difference here has more to do with the allocation path?
>>>>>>>>
>>>>>>>> I went back and reran focusing on the specific migrate type.
>>>>>>>> Without your patch, and coming from the alloc_contig_range call path,
>>>>>>>> I got two warnings of 'page type is 0, passed migratetype is 1' as above.
>>>>>>>> With your patch I got one 'page type is 0, passed migratetype is 1'
>>>>>>>> warning and one 'page type is 1, passed migratetype is 0' warning.
>>>>>>>>
>>>>>>>> I could be wrong, but I do not think your patch changes things.
>>>>>>>
>>>>>>> Got it. Thanks for the clarification.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> One idea about recreating the issue is that it may have to do with size
>>>>>>>>>> of my VM (16G) and the requested allocation sizes 4G. However, I tried
>>>>>>>>>> to really stress the allocations by increasing the number of hugetlb
>>>>>>>>>> pages requested and that did not help. I also noticed that I only seem
>>>>>>>>>> to get two warnings and then they stop, even if I continue to run the
>>>>>>>>>> script.
>>>>>>>>>>
>>>>>>>>>> Zi asked about my config, so it is attached.
>>>>>>>>>
>>>>>>>>> With your config, I still have no luck reproducing the issue. I will keep
>>>>>>>>> trying. Thanks.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Perhaps try running both scripts in parallel?
>>>>>>>
>>>>>>> Yes. It seems to do the trick.
>>>>>>>
>>>>>>>> Adjust the number of hugetlb pages allocated to equal 25% of memory?
>>>>>>>
>>>>>>> I am able to reproduce it with the script below:
>>>>>>>
>>>>>>> while true; do
>>>>>>> echo 4 > /sys/kernel/mm/hugepages/hugepages-1048576kB/nr_hugepages&
>>>>>>> echo 2048 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages&
>>>>>>> wait
>>>>>>> echo 0 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
>>>>>>> echo 0 > /sys/kernel/mm/hugepages/hugepages-1048576kB/nr_hugepages
>>>>>>> done
>>>>>>>
>>>>>>> I will look into the issue.
>>>>>
>>>>> Nice!
>>>>>
>>>>> I managed to reproduce it ONCE, triggering it not even a second after
>>>>> starting the script. But I can't seem to do it twice, even after
>>>>> several reboots and letting it run for minutes.
>>>>
>>>> I managed to reproduce it reliably by cutting the nr_hugepages
>>>> parameters respectively in half.
>>>>
>>>> The one that triggers for me is always MIGRATE_ISOLATE. With some
>>>> printk-tracing, the scenario seems to be this:
>>>>
>>>> #0 #1
>>>> start_isolate_page_range()
>>>> isolate_single_pageblock()
>>>> set_migratetype_isolate(tail)
>>>> lock zone->lock
>>>> move_freepages_block(tail) // nop
>>>> set_pageblock_migratetype(tail)
>>>> unlock zone->lock
>>>> del_page_from_freelist(head)
>>>> expand(head, head_mt)
>>>> WARN(head_mt != tail_mt)
>>>> start_pfn = ALIGN_DOWN(MAX_ORDER_NR_PAGES)
>>>> for (pfn = start_pfn, pfn < end_pfn)
>>>> if (PageBuddy())
>>>> split_free_page(head)
>>>>
>>>> IOW, we update a pageblock that isn't MAX_ORDER aligned, then drop the
>>>> lock. The move_freepages_block() does nothing because the PageBuddy()
>>>> is set on the pageblock to the left. Once we drop the lock, the buddy
>>>> gets allocated and the expand() puts things on the wrong list. The
>>>> splitting code that handles MAX_ORDER blocks runs *after* the tail
>>>> type is set and the lock has been dropped, so it's too late.
>>>
>>> Yes, this is the issue I can confirm as well. But it is intentional to enable
>>> allocating a contiguous range at pageblock granularity instead of MAX_ORDER
>>> granularity. With your changes below, it no longer works, because if there
>>> is an unmovable page in
>>> [ALIGN_DOWN(start_pfn, MAX_ORDER_NR_PAGES), pageblock_start_pfn(start_pfn)),
>>> the allocation fails but it would succeed in current implementation.
>>>
>>> I think a proper fix would be to make move_freepages_block() split the
>>> MAX_ORDER page and put the split pages in the right migratetype free lists.
>>>
>>> I am working on that.
>>
>> After spending half a day on this, I think it is much harder than I thought
>> to get alloc_contig_range() working with the freelist migratetype hygiene
>> patchset. Because alloc_contig_range() relies on racy migratetype changes:
>>
>> 1. pageblocks in the range are first marked as MIGRATE_ISOLATE to prevent
>> another parallel isolation, but they are not moved to the MIGRATE_ISOLATE
>> free list yet.
>>
>> 2. later in the process, isolate_freepages_range() is used to actually grab
>> the free pages.
>>
>> 3. there was no problem when alloc_contig_range() works on MAX_ORDER aligned
>> ranges, since MIGRATE_ISOLATE cannot be set in the middle of free pages or
>> in-use pages. But it is not the case when alloc_contig_range() work on
>> pageblock aligned ranges. Now during isolation phase, free or in-use pages
>> will need to be split to get their subpages into the right free lists.
>>
>> 4. the hardest case is when a in-use page sits across two pageblocks, currently,
>> the code just isolate one pageblock, migrate the page, and let split_free_page()
>> to correct the free list later. But to strictly enforce freelist migratetype
>> hygiene, extra work is needed at free page path to split the free page into
>> the right freelists.
>>
>> I need more time to think about how to get alloc_contig_range() properly.
>> Help is needed for the bullet point 4.
>
>
> I once raised that we should maybe try making MIGRATE_ISOLATE a flag that preserves the original migratetype. Not sure if that would help here in any way.

I have that in my backlog since you asked and have been delaying it. ;) Hopefully
I can do it after I fix this. That change might or might not help only if we make
some redesign on how migratetype is managed. If MIGRATE_ISOLATE does not
overwrite existing migratetype, the code might not need to split a page and move
it to MIGRATE_ISOLATE freelist?

The fundamental issue in alloc_contig_range() is that to work at
pageblock level, a page (>pageblock_order) can have one part is isolated and
the rest is a different migratetype. {add_to,move_to,del_page_from}_free_list()
now checks first pageblock migratetype, so such a page needs to be removed
from its free_list, set MIGRATE_ISOLATE on one of the pageblock, split, and
finally put back to multiple free lists. This needs to be done at isolation stage
before free pages are removed from their free lists (the stage after isolation).
If MIGRATE_ISOLATE is a separate flag and we are OK with leaving isolated pages
in their original migratetype and check migratetype before allocating a page,
that might help. But that might add extra work (e.g., splitting a partially
isolated free page before allocation) in the really hot code path, which is not
desirable.

>
> The whole alloc_contig_range() implementation is quite complicated and hard to grasp. If we could find ways to clean all that up and make it easier to understand and play along, that would be nice.

I will try my best to simplify it.

--
Best Regards,
Yan, Zi

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature

2023-09-26 00:19:10

On 26 Sep 2023, at 13:39, Johannes Weiner wrote:

> On Mon, Sep 25, 2023 at 05:12:38PM -0400, Zi Yan wrote:
>> On 21 Sep 2023, at 10:47, Zi Yan wrote:
>>
>>> On 21 Sep 2023, at 6:19, David Hildenbrand wrote:
>>>
>>>> On 21.09.23 04:31, Zi Yan wrote:
>>>>> On 20 Sep 2023, at 13:23, Zi Yan wrote:
>>>>>
>>>>>> On 20 Sep 2023, at 12:04, Johannes Weiner wrote:
>>>>>>
>>>>>>> On Wed, Sep 20, 2023 at 09:48:12AM -0400, Johannes Weiner wrote:
>>>>>>>> On Wed, Sep 20, 2023 at 08:07:53AM +0200, Vlastimil Babka wrote:
>>>>>>>>> On 9/20/23 03:38, Zi Yan wrote:
>>>>>>>>>> On 19 Sep 2023, at 20:32, Mike Kravetz wrote:
>>>>>>>>>>
>>>>>>>>>>> On 09/19/23 16:57, Zi Yan wrote:
>>>>>>>>>>>> On 19 Sep 2023, at 14:47, Mike Kravetz wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> --- a/mm/page_alloc.c
>>>>>>>>>>>>> +++ b/mm/page_alloc.c
>>>>>>>>>>>>> @@ -1651,8 +1651,13 @@ static bool prep_move_freepages_block(struct zone *zone, struct page *page,
>>>>>>>>>>>>> end = pageblock_end_pfn(pfn) - 1;
>>>>>>>>>>>>>
>>>>>>>>>>>>> /* Do not cross zone boundaries */
>>>>>>>>>>>>> +#if 0
>>>>>>>>>>>>> if (!zone_spans_pfn(zone, start))
>>>>>>>>>>>>> start = zone->zone_start_pfn;
>>>>>>>>>>>>> +#else
>>>>>>>>>>>>> + if (!zone_spans_pfn(zone, start))
>>>>>>>>>>>>> + start = pfn;
>>>>>>>>>>>>> +#endif
>>>>>>>>>>>>> if (!zone_spans_pfn(zone, end))
>>>>>>>>>>>>> return false;
>>>>>>>>>>>>> I can still trigger warnings.
>>>>>>>>>>>>
>>>>>>>>>>>> OK. One thing to note is that the page type in the warning changed from
>>>>>>>>>>>> 5 (MIGRATE_ISOLATE) to 0 (MIGRATE_UNMOVABLE) with my suggested change.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Just to be really clear,
>>>>>>>>>>> - the 5 (MIGRATE_ISOLATE) warning was from the __alloc_pages call path.
>>>>>>>>>>> - the 0 (MIGRATE_UNMOVABLE) as above was from the alloc_contig_range call
>>>>>>>>>>> path WITHOUT your change.
>>>>>>>>>>>
>>>>>>>>>>> I am guessing the difference here has more to do with the allocation path?
>>>>>>>>>>>
>>>>>>>>>>> I went back and reran focusing on the specific migrate type.
>>>>>>>>>>> Without your patch, and coming from the alloc_contig_range call path,
>>>>>>>>>>> I got two warnings of 'page type is 0, passed migratetype is 1' as above.
>>>>>>>>>>> With your patch I got one 'page type is 0, passed migratetype is 1'
>>>>>>>>>>> warning and one 'page type is 1, passed migratetype is 0' warning.
>>>>>>>>>>>
>>>>>>>>>>> I could be wrong, but I do not think your patch changes things.
>>>>>>>>>>
>>>>>>>>>> Got it. Thanks for the clarification.
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> One idea about recreating the issue is that it may have to do with size
>>>>>>>>>>>>> of my VM (16G) and the requested allocation sizes 4G. However, I tried
>>>>>>>>>>>>> to really stress the allocations by increasing the number of hugetlb
>>>>>>>>>>>>> pages requested and that did not help. I also noticed that I only seem
>>>>>>>>>>>>> to get two warnings and then they stop, even if I continue to run the
>>>>>>>>>>>>> script.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Zi asked about my config, so it is attached.
>>>>>>>>>>>>
>>>>>>>>>>>> With your config, I still have no luck reproducing the issue. I will keep
>>>>>>>>>>>> trying. Thanks.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Perhaps try running both scripts in parallel?
>>>>>>>>>>
>>>>>>>>>> Yes. It seems to do the trick.
>>>>>>>>>>
>>>>>>>>>>> Adjust the number of hugetlb pages allocated to equal 25% of memory?
>>>>>>>>>>
>>>>>>>>>> I am able to reproduce it with the script below:
>>>>>>>>>>
>>>>>>>>>> while true; do
>>>>>>>>>> echo 4 > /sys/kernel/mm/hugepages/hugepages-1048576kB/nr_hugepages&
>>>>>>>>>> echo 2048 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages&
>>>>>>>>>> wait
>>>>>>>>>> echo 0 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
>>>>>>>>>> echo 0 > /sys/kernel/mm/hugepages/hugepages-1048576kB/nr_hugepages
>>>>>>>>>> done
>>>>>>>>>>
>>>>>>>>>> I will look into the issue.
>>>>>>>>
>>>>>>>> Nice!
>>>>>>>>
>>>>>>>> I managed to reproduce it ONCE, triggering it not even a second after
>>>>>>>> starting the script. But I can't seem to do it twice, even after
>>>>>>>> several reboots and letting it run for minutes.
>>>>>>>
>>>>>>> I managed to reproduce it reliably by cutting the nr_hugepages
>>>>>>> parameters respectively in half.
>>>>>>>
>>>>>>> The one that triggers for me is always MIGRATE_ISOLATE. With some
>>>>>>> printk-tracing, the scenario seems to be this:
>>>>>>>
>>>>>>> #0 #1
>>>>>>> start_isolate_page_range()
>>>>>>> isolate_single_pageblock()
>>>>>>> set_migratetype_isolate(tail)
>>>>>>> lock zone->lock
>>>>>>> move_freepages_block(tail) // nop
>>>>>>> set_pageblock_migratetype(tail)
>>>>>>> unlock zone->lock
>>>>>>> del_page_from_freelist(head)
>>>>>>> expand(head, head_mt)
>>>>>>> WARN(head_mt != tail_mt)
>>>>>>> start_pfn = ALIGN_DOWN(MAX_ORDER_NR_PAGES)
>>>>>>> for (pfn = start_pfn, pfn < end_pfn)
>>>>>>> if (PageBuddy())
>>>>>>> split_free_page(head)
>>>>>>>
>>>>>>> IOW, we update a pageblock that isn't MAX_ORDER aligned, then drop the
>>>>>>> lock. The move_freepages_block() does nothing because the PageBuddy()
>>>>>>> is set on the pageblock to the left. Once we drop the lock, the buddy
>>>>>>> gets allocated and the expand() puts things on the wrong list. The
>>>>>>> splitting code that handles MAX_ORDER blocks runs *after* the tail
>>>>>>> type is set and the lock has been dropped, so it's too late.
>>>>>>
>>>>>> Yes, this is the issue I can confirm as well. But it is intentional to enable
>>>>>> allocating a contiguous range at pageblock granularity instead of MAX_ORDER
>>>>>> granularity. With your changes below, it no longer works, because if there
>>>>>> is an unmovable page in
>>>>>> [ALIGN_DOWN(start_pfn, MAX_ORDER_NR_PAGES), pageblock_start_pfn(start_pfn)),
>>>>>> the allocation fails but it would succeed in current implementation.
>>>>>>
>>>>>> I think a proper fix would be to make move_freepages_block() split the
>>>>>> MAX_ORDER page and put the split pages in the right migratetype free lists.
>>>>>>
>>>>>> I am working on that.
>>>>>
>>>>> After spending half a day on this, I think it is much harder than I thought
>>>>> to get alloc_contig_range() working with the freelist migratetype hygiene
>>>>> patchset. Because alloc_contig_range() relies on racy migratetype changes:
>>>>>
>>>>> 1. pageblocks in the range are first marked as MIGRATE_ISOLATE to prevent
>>>>> another parallel isolation, but they are not moved to the MIGRATE_ISOLATE
>>>>> free list yet.
>>>>>
>>>>> 2. later in the process, isolate_freepages_range() is used to actually grab
>>>>> the free pages.
>>>>>
>>>>> 3. there was no problem when alloc_contig_range() works on MAX_ORDER aligned
>>>>> ranges, since MIGRATE_ISOLATE cannot be set in the middle of free pages or
>>>>> in-use pages. But it is not the case when alloc_contig_range() work on
>>>>> pageblock aligned ranges. Now during isolation phase, free or in-use pages
>>>>> will need to be split to get their subpages into the right free lists.
>>>>>
>>>>> 4. the hardest case is when a in-use page sits across two pageblocks, currently,
>>>>> the code just isolate one pageblock, migrate the page, and let split_free_page()
>>>>> to correct the free list later. But to strictly enforce freelist migratetype
>>>>> hygiene, extra work is needed at free page path to split the free page into
>>>>> the right freelists.
>>>>>
>>>>> I need more time to think about how to get alloc_contig_range() properly.
>>>>> Help is needed for the bullet point 4.
>>>>
>>>>
>>>> I once raised that we should maybe try making MIGRATE_ISOLATE a flag that preserves the original migratetype. Not sure if that would help here in any way.
>>>
>>> I have that in my backlog since you asked and have been delaying it. ;) Hopefully
>>> I can do it after I fix this. That change might or might not help only if we make
>>> some redesign on how migratetype is managed. If MIGRATE_ISOLATE does not
>>> overwrite existing migratetype, the code might not need to split a page and move
>>> it to MIGRATE_ISOLATE freelist?
>>>
>>> The fundamental issue in alloc_contig_range() is that to work at
>>> pageblock level, a page (>pageblock_order) can have one part is isolated and
>>> the rest is a different migratetype. {add_to,move_to,del_page_from}_free_list()
>>> now checks first pageblock migratetype, so such a page needs to be removed
>>> from its free_list, set MIGRATE_ISOLATE on one of the pageblock, split, and
>>> finally put back to multiple free lists. This needs to be done at isolation stage
>>> before free pages are removed from their free lists (the stage after isolation).
>>> If MIGRATE_ISOLATE is a separate flag and we are OK with leaving isolated pages
>>> in their original migratetype and check migratetype before allocating a page,
>>> that might help. But that might add extra work (e.g., splitting a partially
>>> isolated free page before allocation) in the really hot code path, which is not
>>> desirable.
>>>
>>>>
>>>> The whole alloc_contig_range() implementation is quite complicated and hard to grasp. If we could find ways to clean all that up and make it easier to understand and play along, that would be nice.
>>>
>>> I will try my best to simplify it.
>>
>> Hi Johannes,
>>
>> I attached three patches to fix the issue and first two can be folded into
>> your patchset:
>
> Hi Zi, thanks for providing these patches! I'll pick them up into the
> series.
>
>> 1. __free_one_page() bug you and Vlastimil discussed on the other email.
>> 2. move set_pageblock_migratetype() into move_freepages() to prepare for patch 3.
>> 3. enable move_freepages() to split a free page that is partially covered by
>> [start_pfn, end_pfn] in the parameter and set migratetype correctly when
>> a >pageblock_order free page is moved. Before when a >pageblock_order
>> free page is moved, only first pageblock migratetype is changed. The added
>> WARN_ON_ONCE might be triggered by these pages.
>>
>> I ran Mike's test with transhuge-stress together with my patches on top of your
>> "close migratetype race" patch for more than an hour without any warning.
>> It should unblock your patchset. I will keep working on alloc_contig_range()
>> simplification.
>>
>>
>> --
>> Best Regards,
>> Yan, Zi
>
>> From a18de9a235dc97999fcabdac699f33da9138b0ba Mon Sep 17 00:00:00 2001
>> From: Zi Yan <[email protected]>
>> Date: Fri, 22 Sep 2023 11:11:32 -0400
>> Subject: [PATCH 1/3] mm: fix __free_one_page().
>>
>> Signed-off-by: Zi Yan <[email protected]>
>> ---
>> mm/page_alloc.c | 6 +-----
>> 1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 7de022bc4c7d..72f27d14c8e7 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -787,8 +787,6 @@ static inline void __free_one_page(struct page *page,
>> VM_BUG_ON_PAGE(bad_range(zone, page), page);
>>
>> while (order < MAX_ORDER) {
>> - int buddy_mt;
>> -
>> if (compaction_capture(capc, page, order, migratetype))
>> return;
>>
>> @@ -796,8 +794,6 @@ static inline void __free_one_page(struct page *page,
>> if (!buddy)
>> goto done_merging;
>>
>> - buddy_mt = get_pfnblock_migratetype(buddy, buddy_pfn);
>> -
>> if (unlikely(order >= pageblock_order)) {
>> /*
>> * We want to prevent merge between freepages on pageblock
>> @@ -827,7 +823,7 @@ static inline void __free_one_page(struct page *page,
>> if (page_is_guard(buddy))
>> clear_page_guard(zone, buddy, order);
>> else
>> - del_page_from_free_list(buddy, zone, order, buddy_mt);
>> + del_page_from_free_list(buddy, zone, order, migratetype);
>> combined_pfn = buddy_pfn & pfn;
>> page = page + (combined_pfn - pfn);
>> pfn = combined_pfn;
>
> I had a fix for this that's slightly different. The buddy's type can't
> be changed while it's still on the freelist, so I moved that
> around. The sequence now is:
>
> int buddy_mt = migratetype;
>
> if (unlikely(order >= pageblock_order)) {
> /* This is the only case where buddy_mt can differ */
> buddy_mt = get_pfnblock_migratetype(buddy, buddy_pfn);
> // compat checks...
> }
>
> del_page_from_free_list(buddy, buddy_mt);
>
> if (unlikely(buddy_mt != migratetype))
> set_pageblock_migratetype(buddy, migratetype);
>
>
>> From b11a0e3d8f9d7d91a884c90dc9cebb185c3a2bbc Mon Sep 17 00:00:00 2001
>> From: Zi Yan <[email protected]>
>> Date: Mon, 25 Sep 2023 16:27:14 -0400
>> Subject: [PATCH 2/3] mm: set migratetype after free pages are moved between
>> free lists.
>>
>> This avoids changing migratetype after move_freepages() or
>> move_freepages_block(), which is error prone. It also prepares for upcoming
>> changes to fix move_freepages() not moving free pages partially in the
>> range.
>>
>> Signed-off-by: Zi Yan <[email protected]>
>
> This makes the code much cleaner, thank you!
>
>> From 75a4d327efd94230f3b9aab29ef6ec0badd488a6 Mon Sep 17 00:00:00 2001
>> From: Zi Yan <[email protected]>
>> Date: Mon, 25 Sep 2023 16:55:18 -0400
>> Subject: [PATCH 3/3] mm: enable move_freepages() to properly move part of free
>> pages.
>>
>> alloc_contig_range() uses set_migrateype_isolate(), which eventually calls
>> move_freepages(), to isolate free pages. But move_freepages() was not able
>> to move free pages partially covered by the specified range, leaving a race
>> window open[1]. Fix it by teaching move_freepages() to split a free page
>> when only part of it is going to be moved.
>>
>> In addition, when a >pageblock_order free page is moved, only its first
>> pageblock migratetype is changed. It can cause warnings later. Fix it by
>> set all pageblocks in a free page to the same migratetype after move.
>>
>> split_free_page() is changed to be used in move_freepages() and
>> isolate_single_pageblock(). A common code to find the start pfn of a free
>> page is refactored in get_freepage_start_pfn().
>>
>> [1] https://lore.kernel.org/linux-mm/[email protected]/
>>
>> Signed-off-by: Zi Yan <[email protected]>
>> ---
>> mm/page_alloc.c | 75 ++++++++++++++++++++++++++++++++++++---------
>> mm/page_isolation.c | 17 +++++++---
>> 2 files changed, 73 insertions(+), 19 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 7c41cb5d8a36..3fd5ab40b55c 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -866,15 +866,15 @@ int split_free_page(struct page *free_page,
>> struct zone *zone = page_zone(free_page);
>> unsigned long free_page_pfn = page_to_pfn(free_page);
>> unsigned long pfn;
>> - unsigned long flags;
>> int free_page_order;
>> int mt;
>> int ret = 0;
>>
>> - if (split_pfn_offset == 0)
>> - return ret;
>> + /* zone lock should be held when this function is called */
>> + lockdep_assert_held(&zone->lock);
>>
>> - spin_lock_irqsave(&zone->lock, flags);
>> + if (split_pfn_offset == 0 || split_pfn_offset >= (1 << order))
>> + return ret;
>>
>> if (!PageBuddy(free_page) || buddy_order(free_page) != order) {
>> ret = -ENOENT;
>> @@ -900,7 +900,6 @@ int split_free_page(struct page *free_page,
>> split_pfn_offset = (1UL << order) - (pfn - free_page_pfn);
>> }
>> out:
>> - spin_unlock_irqrestore(&zone->lock, flags);
>> return ret;
>> }
>> /*
>> @@ -1589,6 +1588,25 @@ static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
>> unsigned int order) { return NULL; }
>> #endif
>>
>> +/*
>> + * Get first pfn of the free page, where pfn is in. If this free page does
>> + * not exist, return the given pfn.
>> + */
>> +static unsigned long get_freepage_start_pfn(unsigned long pfn)
>> +{
>> + int order = 0;
>> + unsigned long start_pfn = pfn;
>> +
>> + while (!PageBuddy(pfn_to_page(start_pfn))) {
>> + if (++order > MAX_ORDER) {
>> + start_pfn = pfn;
>> + break;
>> + }
>> + start_pfn &= ~0UL << order;
>> + }
>> + return start_pfn;
>> +}
>> +
>> /*
>> * Move the free pages in a range to the freelist tail of the requested type.
>> * Note that start_page and end_pages are not aligned on a pageblock
>> @@ -1598,9 +1616,29 @@ static int move_freepages(struct zone *zone, unsigned long start_pfn,
>> unsigned long end_pfn, int old_mt, int new_mt)
>> {
>> struct page *page;
>> - unsigned long pfn;
>> + unsigned long pfn, pfn2;
>> unsigned int order;
>> int pages_moved = 0;
>> + unsigned long mt_change_pfn = start_pfn;
>> + unsigned long new_start_pfn = get_freepage_start_pfn(start_pfn);
>> +
>> + /* split at start_pfn if it is in the middle of a free page */
>> + if (new_start_pfn != start_pfn && PageBuddy(pfn_to_page(new_start_pfn))) {
>> + struct page *new_page = pfn_to_page(new_start_pfn);
>> + int new_page_order = buddy_order(new_page);
>> +
>> + if (new_start_pfn + (1 << new_page_order) > start_pfn) {
>> + /* change migratetype so that split_free_page can work */
>> + set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>> + split_free_page(new_page, buddy_order(new_page),
>> + start_pfn - new_start_pfn);
>> +
>> + mt_change_pfn = start_pfn;
>> + /* move to next page */
>> + start_pfn = new_start_pfn + (1 << new_page_order);
>> + }
>> + }
>
> Ok, so if there is a straddle from the previous block into our block
> of interest, it's split and the migratetype is set only on our block.

Correct. For example, start_pfn is 0x200 (2MB) and the free page starting from 0x0 is order-10 (4MB).

>
>> @@ -1615,10 +1653,24 @@ 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 for all pageblocks within the page and
>> + * only after we move all free pages in one pageblock
>> + */
>> + if (pfn + (1 << order) >= pageblock_end_pfn(pfn)) {
>> + for (pfn2 = pfn; pfn2 < pfn + (1 << order);
>> + pfn2 += pageblock_nr_pages) {
>> + set_pageblock_migratetype(pfn_to_page(pfn2),
>> + new_mt);
>> + mt_change_pfn = pfn2;
>> + }
>
> But if we have the first block of a MAX_ORDER chunk, then we don't
> split but rather move the whole chunk and make sure to update the
> chunk's blocks that are outside the range of interest.
>
> It looks like either way would work, but why not split here as well
> and keep the move contained to the block? Wouldn't this be a bit more
> predictable and easier to understand?

Yes, having a split here would be consistent.

Also I want to spell out the corner case I am handling here (and I will add
it to the comment): since move_to_free_list() checks page's 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 that are
in the same pageblock, setting migratetype right after move_to_free_list()
triggers the warning.

>> + }
>> pfn += 1 << order;
>> pages_moved += 1 << order;
>> }
>> - set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>> + /* set migratetype for the remaining pageblocks */
>> + for (pfn2 = mt_change_pfn; pfn2 <= end_pfn; pfn2 += pageblock_nr_pages)
>> + set_pageblock_migratetype(pfn_to_page(pfn2), new_mt);
>
> I think I'm missing something for this.
>
> - If there was no straddle, there is only our block of interest to
> update.
>
> - If there was a straddle from the previous block, it was split and
> the block of interest was already updated. Nothing to do here?
>
> - If there was a straddle into the next block, both blocks are updated
> to the new type. Nothing to do here?
>
> What's the case where there are multiple blocks to update in the end?

When a pageblock has free pages at the beginning and in-use pages at the end.
The pageblock migratetype is not changed in the for loop above, since free
pages do not cross pageblock boundary. But these free pages are moved
to a new mt free list and will trigger warnings later.

Also if multiple pageblocks are filled with only in-use pages, the for loop
does nothing either. Their pageblocks will be set at this moment. I notice
it might be a change of behavior as I am writing, but this change might
be better. Before, in-page migrateype might or might not be changed,
depending on if there is a free page in the same pageblock or not, meaning
there will be migratetype holes in the specified range. Now the whole range
is changed to new_mt. Let me know if you have a different opinion.

>> @@ -380,8 +380,15 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>> int order = buddy_order(page);
>>
>> if (pfn + (1UL << order) > boundary_pfn) {
>> + int res;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&zone->lock, flags);
>> + res = split_free_page(page, order, boundary_pfn - pfn);
>> + spin_unlock_irqrestore(&zone->lock, flags);
>> +
>> /* free page changed before split, check it again */
>> - if (split_free_page(page, order, boundary_pfn - pfn))
>> + if (res)
>> continue;
>
> At this point, we've already set the migratetype, which has handled
> straddling free pages. Is this split still needed?

Good point. I will remove it. Originally, I thought it should stay to handle
the free page coming from the migration below. But unless a greater than pageblock
order in-use page shows up in the system and it is freed directly via __free_pages(),
any free page coming from the migration below should be put in the right
free list.

Such > pageblock order pages are possible, only if we have >PMD order THPs
or __PageMovable. IIRC, both do not exist yet.

>
>> @@ -426,9 +433,11 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>> /*
>> * XXX: mark the page as MIGRATE_ISOLATE so that
>> * no one else can grab the freed page after migration.
>> - * Ideally, the page should be freed as two separate
>> - * pages to be added into separate migratetype free
>> - * lists.
>> + * The page should be freed into separate migratetype
>> + * free lists, unless the free page order is greater
>> + * than pageblock order. It is not the case now,
>> + * since gigantic hugetlb is freed as order-0
>> + * pages and LRU pages do not cross pageblocks.
>> */
>> if (isolate_page) {
>> ret = set_migratetype_isolate(page, page_mt,
>
> I hadn't thought about LRU pages being constrained to single
> pageblocks before. Does this mean we only ever migrate here in case

Initially, I thought a lot about what if a high order folio crosses
two adjacent pageblocks, but at the end I find that __find_buddy_pfn()
does not treat pfns from adjacent pageblocks as buddy unless order
is greater than pageblock order. So any high order folio from
buddy allocator does not cross pageblocks. That is a relief.

Another (future) possibility is once anon large folio is merged and
my split huge page to any lower order patches are merged, a high order
folio might not come directly from buddy allocator but from a huge page
split. But that requires a > pageblock order folio exist first, which
is not possible either. So we are good.

> there is a movable gigantic page? And since those are already split
> during the free, does that mean the "reset pfn to head of the free
> page" part after the migration is actually unnecessary?

Yes. the "reset pfn" code could be removed.

Thank you for the review. Really appreciate it. Let me revise my
patch 3 and send it out again.

--
Best Regards,
Yan, Zi

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature

2023-09-28 15:34:03

On 27 Sep 2023, at 22:51, Zi Yan wrote:

> On 26 Sep 2023, at 13:39, Johannes Weiner wrote:
>
>> On Mon, Sep 25, 2023 at 05:12:38PM -0400, Zi Yan wrote:
>>> On 21 Sep 2023, at 10:47, Zi Yan wrote:
>>>
>>>> On 21 Sep 2023, at 6:19, David Hildenbrand wrote:
>>>>
>>>>> On 21.09.23 04:31, Zi Yan wrote:
>>>>>> On 20 Sep 2023, at 13:23, Zi Yan wrote:
>>>>>>
>>>>>>> On 20 Sep 2023, at 12:04, Johannes Weiner wrote:
>>>>>>>
>>>>>>>> On Wed, Sep 20, 2023 at 09:48:12AM -0400, Johannes Weiner wrote:
>>>>>>>>> On Wed, Sep 20, 2023 at 08:07:53AM +0200, Vlastimil Babka wrote:
>>>>>>>>>> On 9/20/23 03:38, Zi Yan wrote:
>>>>>>>>>>> On 19 Sep 2023, at 20:32, Mike Kravetz wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On 09/19/23 16:57, Zi Yan wrote:
>>>>>>>>>>>>> On 19 Sep 2023, at 14:47, Mike Kravetz wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> --- a/mm/page_alloc.c
>>>>>>>>>>>>>> +++ b/mm/page_alloc.c
>>>>>>>>>>>>>> @@ -1651,8 +1651,13 @@ static bool prep_move_freepages_block(struct zone *zone, struct page *page,
>>>>>>>>>>>>>> end = pageblock_end_pfn(pfn) - 1;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /* Do not cross zone boundaries */
>>>>>>>>>>>>>> +#if 0
>>>>>>>>>>>>>> if (!zone_spans_pfn(zone, start))
>>>>>>>>>>>>>> start = zone->zone_start_pfn;
>>>>>>>>>>>>>> +#else
>>>>>>>>>>>>>> + if (!zone_spans_pfn(zone, start))
>>>>>>>>>>>>>> + start = pfn;
>>>>>>>>>>>>>> +#endif
>>>>>>>>>>>>>> if (!zone_spans_pfn(zone, end))
>>>>>>>>>>>>>> return false;
>>>>>>>>>>>>>> I can still trigger warnings.
>>>>>>>>>>>>>
>>>>>>>>>>>>> OK. One thing to note is that the page type in the warning changed from
>>>>>>>>>>>>> 5 (MIGRATE_ISOLATE) to 0 (MIGRATE_UNMOVABLE) with my suggested change.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Just to be really clear,
>>>>>>>>>>>> - the 5 (MIGRATE_ISOLATE) warning was from the __alloc_pages call path.
>>>>>>>>>>>> - the 0 (MIGRATE_UNMOVABLE) as above was from the alloc_contig_range call
>>>>>>>>>>>> path WITHOUT your change.
>>>>>>>>>>>>
>>>>>>>>>>>> I am guessing the difference here has more to do with the allocation path?
>>>>>>>>>>>>
>>>>>>>>>>>> I went back and reran focusing on the specific migrate type.
>>>>>>>>>>>> Without your patch, and coming from the alloc_contig_range call path,
>>>>>>>>>>>> I got two warnings of 'page type is 0, passed migratetype is 1' as above.
>>>>>>>>>>>> With your patch I got one 'page type is 0, passed migratetype is 1'
>>>>>>>>>>>> warning and one 'page type is 1, passed migratetype is 0' warning.
>>>>>>>>>>>>
>>>>>>>>>>>> I could be wrong, but I do not think your patch changes things.
>>>>>>>>>>>
>>>>>>>>>>> Got it. Thanks for the clarification.
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> One idea about recreating the issue is that it may have to do with size
>>>>>>>>>>>>>> of my VM (16G) and the requested allocation sizes 4G. However, I tried
>>>>>>>>>>>>>> to really stress the allocations by increasing the number of hugetlb
>>>>>>>>>>>>>> pages requested and that did not help. I also noticed that I only seem
>>>>>>>>>>>>>> to get two warnings and then they stop, even if I continue to run the
>>>>>>>>>>>>>> script.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Zi asked about my config, so it is attached.
>>>>>>>>>>>>>
>>>>>>>>>>>>> With your config, I still have no luck reproducing the issue. I will keep
>>>>>>>>>>>>> trying. Thanks.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Perhaps try running both scripts in parallel?
>>>>>>>>>>>
>>>>>>>>>>> Yes. It seems to do the trick.
>>>>>>>>>>>
>>>>>>>>>>>> Adjust the number of hugetlb pages allocated to equal 25% of memory?
>>>>>>>>>>>
>>>>>>>>>>> I am able to reproduce it with the script below:
>>>>>>>>>>>
>>>>>>>>>>> while true; do
>>>>>>>>>>> echo 4 > /sys/kernel/mm/hugepages/hugepages-1048576kB/nr_hugepages&
>>>>>>>>>>> echo 2048 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages&
>>>>>>>>>>> wait
>>>>>>>>>>> echo 0 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
>>>>>>>>>>> echo 0 > /sys/kernel/mm/hugepages/hugepages-1048576kB/nr_hugepages
>>>>>>>>>>> done
>>>>>>>>>>>
>>>>>>>>>>> I will look into the issue.
>>>>>>>>>
>>>>>>>>> Nice!
>>>>>>>>>
>>>>>>>>> I managed to reproduce it ONCE, triggering it not even a second after
>>>>>>>>> starting the script. But I can't seem to do it twice, even after
>>>>>>>>> several reboots and letting it run for minutes.
>>>>>>>>
>>>>>>>> I managed to reproduce it reliably by cutting the nr_hugepages
>>>>>>>> parameters respectively in half.
>>>>>>>>
>>>>>>>> The one that triggers for me is always MIGRATE_ISOLATE. With some
>>>>>>>> printk-tracing, the scenario seems to be this:
>>>>>>>>
>>>>>>>> #0 #1
>>>>>>>> start_isolate_page_range()
>>>>>>>> isolate_single_pageblock()
>>>>>>>> set_migratetype_isolate(tail)
>>>>>>>> lock zone->lock
>>>>>>>> move_freepages_block(tail) // nop
>>>>>>>> set_pageblock_migratetype(tail)
>>>>>>>> unlock zone->lock
>>>>>>>> del_page_from_freelist(head)
>>>>>>>> expand(head, head_mt)
>>>>>>>> WARN(head_mt != tail_mt)
>>>>>>>> start_pfn = ALIGN_DOWN(MAX_ORDER_NR_PAGES)
>>>>>>>> for (pfn = start_pfn, pfn < end_pfn)
>>>>>>>> if (PageBuddy())
>>>>>>>> split_free_page(head)
>>>>>>>>
>>>>>>>> IOW, we update a pageblock that isn't MAX_ORDER aligned, then drop the
>>>>>>>> lock. The move_freepages_block() does nothing because the PageBuddy()
>>>>>>>> is set on the pageblock to the left. Once we drop the lock, the buddy
>>>>>>>> gets allocated and the expand() puts things on the wrong list. The
>>>>>>>> splitting code that handles MAX_ORDER blocks runs *after* the tail
>>>>>>>> type is set and the lock has been dropped, so it's too late.
>>>>>>>
>>>>>>> Yes, this is the issue I can confirm as well. But it is intentional to enable
>>>>>>> allocating a contiguous range at pageblock granularity instead of MAX_ORDER
>>>>>>> granularity. With your changes below, it no longer works, because if there
>>>>>>> is an unmovable page in
>>>>>>> [ALIGN_DOWN(start_pfn, MAX_ORDER_NR_PAGES), pageblock_start_pfn(start_pfn)),
>>>>>>> the allocation fails but it would succeed in current implementation.
>>>>>>>
>>>>>>> I think a proper fix would be to make move_freepages_block() split the
>>>>>>> MAX_ORDER page and put the split pages in the right migratetype free lists.
>>>>>>>
>>>>>>> I am working on that.
>>>>>>
>>>>>> After spending half a day on this, I think it is much harder than I thought
>>>>>> to get alloc_contig_range() working with the freelist migratetype hygiene
>>>>>> patchset. Because alloc_contig_range() relies on racy migratetype changes:
>>>>>>
>>>>>> 1. pageblocks in the range are first marked as MIGRATE_ISOLATE to prevent
>>>>>> another parallel isolation, but they are not moved to the MIGRATE_ISOLATE
>>>>>> free list yet.
>>>>>>
>>>>>> 2. later in the process, isolate_freepages_range() is used to actually grab
>>>>>> the free pages.
>>>>>>
>>>>>> 3. there was no problem when alloc_contig_range() works on MAX_ORDER aligned
>>>>>> ranges, since MIGRATE_ISOLATE cannot be set in the middle of free pages or
>>>>>> in-use pages. But it is not the case when alloc_contig_range() work on
>>>>>> pageblock aligned ranges. Now during isolation phase, free or in-use pages
>>>>>> will need to be split to get their subpages into the right free lists.
>>>>>>
>>>>>> 4. the hardest case is when a in-use page sits across two pageblocks, currently,
>>>>>> the code just isolate one pageblock, migrate the page, and let split_free_page()
>>>>>> to correct the free list later. But to strictly enforce freelist migratetype
>>>>>> hygiene, extra work is needed at free page path to split the free page into
>>>>>> the right freelists.
>>>>>>
>>>>>> I need more time to think about how to get alloc_contig_range() properly.
>>>>>> Help is needed for the bullet point 4.
>>>>>
>>>>>
>>>>> I once raised that we should maybe try making MIGRATE_ISOLATE a flag that preserves the original migratetype. Not sure if that would help here in any way.
>>>>
>>>> I have that in my backlog since you asked and have been delaying it. ;) Hopefully
>>>> I can do it after I fix this. That change might or might not help only if we make
>>>> some redesign on how migratetype is managed. If MIGRATE_ISOLATE does not
>>>> overwrite existing migratetype, the code might not need to split a page and move
>>>> it to MIGRATE_ISOLATE freelist?
>>>>
>>>> The fundamental issue in alloc_contig_range() is that to work at
>>>> pageblock level, a page (>pageblock_order) can have one part is isolated and
>>>> the rest is a different migratetype. {add_to,move_to,del_page_from}_free_list()
>>>> now checks first pageblock migratetype, so such a page needs to be removed
>>>> from its free_list, set MIGRATE_ISOLATE on one of the pageblock, split, and
>>>> finally put back to multiple free lists. This needs to be done at isolation stage
>>>> before free pages are removed from their free lists (the stage after isolation).
>>>> If MIGRATE_ISOLATE is a separate flag and we are OK with leaving isolated pages
>>>> in their original migratetype and check migratetype before allocating a page,
>>>> that might help. But that might add extra work (e.g., splitting a partially
>>>> isolated free page before allocation) in the really hot code path, which is not
>>>> desirable.
>>>>
>>>>>
>>>>> The whole alloc_contig_range() implementation is quite complicated and hard to grasp. If we could find ways to clean all that up and make it easier to understand and play along, that would be nice.
>>>>
>>>> I will try my best to simplify it.
>>>
>>> Hi Johannes,
>>>
>>> I attached three patches to fix the issue and first two can be folded into
>>> your patchset:
>>
>> Hi Zi, thanks for providing these patches! I'll pick them up into the
>> series.
>>
>>> 1. __free_one_page() bug you and Vlastimil discussed on the other email.
>>> 2. move set_pageblock_migratetype() into move_freepages() to prepare for patch 3.
>>> 3. enable move_freepages() to split a free page that is partially covered by
>>> [start_pfn, end_pfn] in the parameter and set migratetype correctly when
>>> a >pageblock_order free page is moved. Before when a >pageblock_order
>>> free page is moved, only first pageblock migratetype is changed. The added
>>> WARN_ON_ONCE might be triggered by these pages.
>>>
>>> I ran Mike's test with transhuge-stress together with my patches on top of your
>>> "close migratetype race" patch for more than an hour without any warning.
>>> It should unblock your patchset. I will keep working on alloc_contig_range()
>>> simplification.
>>>
>>>
>>> --
>>> Best Regards,
>>> Yan, Zi
>>
>>> From a18de9a235dc97999fcabdac699f33da9138b0ba Mon Sep 17 00:00:00 2001
>>> From: Zi Yan <[email protected]>
>>> Date: Fri, 22 Sep 2023 11:11:32 -0400
>>> Subject: [PATCH 1/3] mm: fix __free_one_page().
>>>
>>> Signed-off-by: Zi Yan <[email protected]>
>>> ---
>>> mm/page_alloc.c | 6 +-----
>>> 1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 7de022bc4c7d..72f27d14c8e7 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -787,8 +787,6 @@ static inline void __free_one_page(struct page *page,
>>> VM_BUG_ON_PAGE(bad_range(zone, page), page);
>>>
>>> while (order < MAX_ORDER) {
>>> - int buddy_mt;
>>> -
>>> if (compaction_capture(capc, page, order, migratetype))
>>> return;
>>>
>>> @@ -796,8 +794,6 @@ static inline void __free_one_page(struct page *page,
>>> if (!buddy)
>>> goto done_merging;
>>>
>>> - buddy_mt = get_pfnblock_migratetype(buddy, buddy_pfn);
>>> -
>>> if (unlikely(order >= pageblock_order)) {
>>> /*
>>> * We want to prevent merge between freepages on pageblock
>>> @@ -827,7 +823,7 @@ static inline void __free_one_page(struct page *page,
>>> if (page_is_guard(buddy))
>>> clear_page_guard(zone, buddy, order);
>>> else
>>> - del_page_from_free_list(buddy, zone, order, buddy_mt);
>>> + del_page_from_free_list(buddy, zone, order, migratetype);
>>> combined_pfn = buddy_pfn & pfn;
>>> page = page + (combined_pfn - pfn);
>>> pfn = combined_pfn;
>>
>> I had a fix for this that's slightly different. The buddy's type can't
>> be changed while it's still on the freelist, so I moved that
>> around. The sequence now is:
>>
>> int buddy_mt = migratetype;
>>
>> if (unlikely(order >= pageblock_order)) {
>> /* This is the only case where buddy_mt can differ */
>> buddy_mt = get_pfnblock_migratetype(buddy, buddy_pfn);
>> // compat checks...
>> }
>>
>> del_page_from_free_list(buddy, buddy_mt);
>>
>> if (unlikely(buddy_mt != migratetype))
>> set_pageblock_migratetype(buddy, migratetype);
>>
>>
>>> From b11a0e3d8f9d7d91a884c90dc9cebb185c3a2bbc Mon Sep 17 00:00:00 2001
>>> From: Zi Yan <[email protected]>
>>> Date: Mon, 25 Sep 2023 16:27:14 -0400
>>> Subject: [PATCH 2/3] mm: set migratetype after free pages are moved between
>>> free lists.
>>>
>>> This avoids changing migratetype after move_freepages() or
>>> move_freepages_block(), which is error prone. It also prepares for upcoming
>>> changes to fix move_freepages() not moving free pages partially in the
>>> range.
>>>
>>> Signed-off-by: Zi Yan <[email protected]>
>>
>> This makes the code much cleaner, thank you!
>>
>>> From 75a4d327efd94230f3b9aab29ef6ec0badd488a6 Mon Sep 17 00:00:00 2001
>>> From: Zi Yan <[email protected]>
>>> Date: Mon, 25 Sep 2023 16:55:18 -0400
>>> Subject: [PATCH 3/3] mm: enable move_freepages() to properly move part of free
>>> pages.
>>>
>>> alloc_contig_range() uses set_migrateype_isolate(), which eventually calls
>>> move_freepages(), to isolate free pages. But move_freepages() was not able
>>> to move free pages partially covered by the specified range, leaving a race
>>> window open[1]. Fix it by teaching move_freepages() to split a free page
>>> when only part of it is going to be moved.
>>>
>>> In addition, when a >pageblock_order free page is moved, only its first
>>> pageblock migratetype is changed. It can cause warnings later. Fix it by
>>> set all pageblocks in a free page to the same migratetype after move.
>>>
>>> split_free_page() is changed to be used in move_freepages() and
>>> isolate_single_pageblock(). A common code to find the start pfn of a free
>>> page is refactored in get_freepage_start_pfn().
>>>
>>> [1] https://lore.kernel.org/linux-mm/[email protected]/
>>>
>>> Signed-off-by: Zi Yan <[email protected]>
>>> ---
>>> mm/page_alloc.c | 75 ++++++++++++++++++++++++++++++++++++---------
>>> mm/page_isolation.c | 17 +++++++---
>>> 2 files changed, 73 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 7c41cb5d8a36..3fd5ab40b55c 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -866,15 +866,15 @@ int split_free_page(struct page *free_page,
>>> struct zone *zone = page_zone(free_page);
>>> unsigned long free_page_pfn = page_to_pfn(free_page);
>>> unsigned long pfn;
>>> - unsigned long flags;
>>> int free_page_order;
>>> int mt;
>>> int ret = 0;
>>>
>>> - if (split_pfn_offset == 0)
>>> - return ret;
>>> + /* zone lock should be held when this function is called */
>>> + lockdep_assert_held(&zone->lock);
>>>
>>> - spin_lock_irqsave(&zone->lock, flags);
>>> + if (split_pfn_offset == 0 || split_pfn_offset >= (1 << order))
>>> + return ret;
>>>
>>> if (!PageBuddy(free_page) || buddy_order(free_page) != order) {
>>> ret = -ENOENT;
>>> @@ -900,7 +900,6 @@ int split_free_page(struct page *free_page,
>>> split_pfn_offset = (1UL << order) - (pfn - free_page_pfn);
>>> }
>>> out:
>>> - spin_unlock_irqrestore(&zone->lock, flags);
>>> return ret;
>>> }
>>> /*
>>> @@ -1589,6 +1588,25 @@ static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
>>> unsigned int order) { return NULL; }
>>> #endif
>>>
>>> +/*
>>> + * Get first pfn of the free page, where pfn is in. If this free page does
>>> + * not exist, return the given pfn.
>>> + */
>>> +static unsigned long get_freepage_start_pfn(unsigned long pfn)
>>> +{
>>> + int order = 0;
>>> + unsigned long start_pfn = pfn;
>>> +
>>> + while (!PageBuddy(pfn_to_page(start_pfn))) {
>>> + if (++order > MAX_ORDER) {
>>> + start_pfn = pfn;
>>> + break;
>>> + }
>>> + start_pfn &= ~0UL << order;
>>> + }
>>> + return start_pfn;
>>> +}
>>> +
>>> /*
>>> * Move the free pages in a range to the freelist tail of the requested type.
>>> * Note that start_page and end_pages are not aligned on a pageblock
>>> @@ -1598,9 +1616,29 @@ static int move_freepages(struct zone *zone, unsigned long start_pfn,
>>> unsigned long end_pfn, int old_mt, int new_mt)
>>> {
>>> struct page *page;
>>> - unsigned long pfn;
>>> + unsigned long pfn, pfn2;
>>> unsigned int order;
>>> int pages_moved = 0;
>>> + unsigned long mt_change_pfn = start_pfn;
>>> + unsigned long new_start_pfn = get_freepage_start_pfn(start_pfn);
>>> +
>>> + /* split at start_pfn if it is in the middle of a free page */
>>> + if (new_start_pfn != start_pfn && PageBuddy(pfn_to_page(new_start_pfn))) {
>>> + struct page *new_page = pfn_to_page(new_start_pfn);
>>> + int new_page_order = buddy_order(new_page);
>>> +
>>> + if (new_start_pfn + (1 << new_page_order) > start_pfn) {
>>> + /* change migratetype so that split_free_page can work */
>>> + set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>>> + split_free_page(new_page, buddy_order(new_page),
>>> + start_pfn - new_start_pfn);
>>> +
>>> + mt_change_pfn = start_pfn;
>>> + /* move to next page */
>>> + start_pfn = new_start_pfn + (1 << new_page_order);
>>> + }
>>> + }
>>
>> Ok, so if there is a straddle from the previous block into our block
>> of interest, it's split and the migratetype is set only on our block.
>
> Correct. For example, start_pfn is 0x200 (2MB) and the free page starting from 0x0 is order-10 (4MB).
>
>>
>>> @@ -1615,10 +1653,24 @@ 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 for all pageblocks within the page and
>>> + * only after we move all free pages in one pageblock
>>> + */
>>> + if (pfn + (1 << order) >= pageblock_end_pfn(pfn)) {
>>> + for (pfn2 = pfn; pfn2 < pfn + (1 << order);
>>> + pfn2 += pageblock_nr_pages) {
>>> + set_pageblock_migratetype(pfn_to_page(pfn2),
>>> + new_mt);
>>> + mt_change_pfn = pfn2;
>>> + }
>>
>> But if we have the first block of a MAX_ORDER chunk, then we don't
>> split but rather move the whole chunk and make sure to update the
>> chunk's blocks that are outside the range of interest.
>>
>> It looks like either way would work, but why not split here as well
>> and keep the move contained to the block? Wouldn't this be a bit more
>> predictable and easier to understand?
>
> Yes, having a split here would be consistent.
>
> Also I want to spell out the corner case I am handling here (and I will add
> it to the comment): since move_to_free_list() checks page's 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 that are
> in the same pageblock, setting migratetype right after move_to_free_list()
> triggers the warning.
>
>>> + }
>>> pfn += 1 << order;
>>> pages_moved += 1 << order;
>>> }
>>> - set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>>> + /* set migratetype for the remaining pageblocks */
>>> + for (pfn2 = mt_change_pfn; pfn2 <= end_pfn; pfn2 += pageblock_nr_pages)
>>> + set_pageblock_migratetype(pfn_to_page(pfn2), new_mt);
>>
>> I think I'm missing something for this.
>>
>> - If there was no straddle, there is only our block of interest to
>> update.
>>
>> - If there was a straddle from the previous block, it was split and
>> the block of interest was already updated. Nothing to do here?
>>
>> - If there was a straddle into the next block, both blocks are updated
>> to the new type. Nothing to do here?
>>
>> What's the case where there are multiple blocks to update in the end?
>
> When a pageblock has free pages at the beginning and in-use pages at the end.
> The pageblock migratetype is not changed in the for loop above, since free
> pages do not cross pageblock boundary. But these free pages are moved
> to a new mt free list and will trigger warnings later.
>
> Also if multiple pageblocks are filled with only in-use pages, the for loop
> does nothing either. Their pageblocks will be set at this moment. I notice
> it might be a change of behavior as I am writing, but this change might
> be better. Before, in-page migrateype might or might not be changed,
> depending on if there is a free page in the same pageblock or not, meaning
> there will be migratetype holes in the specified range. Now the whole range
> is changed to new_mt. Let me know if you have a different opinion.
>
>
>>> @@ -380,8 +380,15 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>>> int order = buddy_order(page);
>>>
>>> if (pfn + (1UL << order) > boundary_pfn) {
>>> + int res;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&zone->lock, flags);
>>> + res = split_free_page(page, order, boundary_pfn - pfn);
>>> + spin_unlock_irqrestore(&zone->lock, flags);
>>> +
>>> /* free page changed before split, check it again */
>>> - if (split_free_page(page, order, boundary_pfn - pfn))
>>> + if (res)
>>> continue;
>>
>> At this point, we've already set the migratetype, which has handled
>> straddling free pages. Is this split still needed?
>
> Good point. I will remove it. Originally, I thought it should stay to handle
> the free page coming from the migration below. But unless a greater than pageblock
> order in-use page shows up in the system and it is freed directly via __free_pages(),
> any free page coming from the migration below should be put in the right
> free list.
>
> Such > pageblock order pages are possible, only if we have >PMD order THPs
> or __PageMovable. IIRC, both do not exist yet.
>
>>
>>> @@ -426,9 +433,11 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>>> /*
>>> * XXX: mark the page as MIGRATE_ISOLATE so that
>>> * no one else can grab the freed page after migration.
>>> - * Ideally, the page should be freed as two separate
>>> - * pages to be added into separate migratetype free
>>> - * lists.
>>> + * The page should be freed into separate migratetype
>>> + * free lists, unless the free page order is greater
>>> + * than pageblock order. It is not the case now,
>>> + * since gigantic hugetlb is freed as order-0
>>> + * pages and LRU pages do not cross pageblocks.
>>> */
>>> if (isolate_page) {
>>> ret = set_migratetype_isolate(page, page_mt,
>>
>> I hadn't thought about LRU pages being constrained to single
>> pageblocks before. Does this mean we only ever migrate here in case
>
> Initially, I thought a lot about what if a high order folio crosses
> two adjacent pageblocks, but at the end I find that __find_buddy_pfn()
> does not treat pfns from adjacent pageblocks as buddy unless order
> is greater than pageblock order. So any high order folio from
> buddy allocator does not cross pageblocks. That is a relief.
>
> Another (future) possibility is once anon large folio is merged and
> my split huge page to any lower order patches are merged, a high order
> folio might not come directly from buddy allocator but from a huge page
> split. But that requires a > pageblock order folio exist first, which
> is not possible either. So we are good.
>
>> there is a movable gigantic page? And since those are already split
>> during the free, does that mean the "reset pfn to head of the free
>> page" part after the migration is actually unnecessary?
>
> Yes. the "reset pfn" code could be removed.
>
> Thank you for the review. Really appreciate it. Let me revise my
> patch 3 and send it out again.

It turns out that there was a bug in my patch 2: set_pageblock_migratetype()
is used by isolated_page case too, thus cannot be removed unconditionally.

I attached my revised patch 2 and 3 (with all the suggestions above).

--
Best Regards,
Yan, Zi

Attachments:

v2-0001-mm-set-migratetype-after-free-pages-are-moved-bet.patch (3.38 kB)
v2-0002-mm-enable-move_freepages-to-properly-move-part-of.patch (8.83 kB)
signature.asc (871.00 B)
OpenPGP digital signature Download all attachments

2023-10-03 02:36:13

On Mon, Oct 16, 2023 at 09:35:34AM -0400, Zi Yan wrote:
> > The attached patch has all the suggested changes, let me know how it
> > looks to you. Thanks.
>
> The one I sent has free page accounting issues. The attached one fixes them.

Do you still have the warnings? I wonder what went wrong.

> @@ -883,6 +886,10 @@ int split_free_page(struct page *free_page,
> mt = get_pfnblock_migratetype(free_page, free_page_pfn);
> del_page_from_free_list(free_page, zone, order, mt);
>
> + set_pageblock_migratetype(free_page, mt1);
> + set_pageblock_migratetype(pfn_to_page(free_page_pfn + split_pfn_offset),
> + mt2);
> +
> for (pfn = free_page_pfn;
> pfn < free_page_pfn + (1UL << order);) {
> int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);

I don't think this is quite right.

With CONFIG_ARCH_FORCE_MAX_ORDER it's possible that we're dealing with
a buddy that is more than two blocks:

[pageblock 0][pageblock 1][pageblock 2][pageblock 3]
[buddy ]
[isolate range ..

That for loop splits the buddy into 4 blocks. The code above would set
pageblock 0 to old_mt, and pageblock 1 to new_mt. But it should only
set pageblock 3 to new_mt.

My proposal had the mt update in the caller:

> @@ -139,6 +139,62 @@ static struct page *has_unmovable_pages(unsigned long start_pfn, unsigned long e
> return NULL;
> }
>
> +/*
> + * additional steps for moving free pages during page isolation
> + */
> +static int move_freepages_for_isolation(struct zone *zone, unsigned long start_pfn,
> + unsigned long end_pfn, int old_mt, int new_mt)
> +{
> + struct page *start_page = pfn_to_page(start_pfn);
> + unsigned long pfn;
> +
> + VM_WARN_ON(start_pfn & (pageblock_nr_pages - 1));
> + VM_WARN_ON(start_pfn + pageblock_nr_pages - 1 != end_pfn);
> +
> + /*
> + * A free page may be comprised of 2^n blocks, which means our
> + * block of interest could be head or tail in such a page.
> + *
> + * If we're a tail, update the type of our block, then split
> + * the page into pageblocks. The splitting will do the leg
> + * work of sorting the blocks into the right freelists.
> + *
> + * If we're a head, split the page into pageblocks first. This
> + * ensures the migratetypes still match up during the freelist
> + * removal. Then do the regular scan for buddies in the block
> + * of interest, which will handle the rest.
> + *
> + * In theory, we could try to preserve 2^1 and larger blocks
> + * that lie outside our range. In practice, MAX_ORDER is
> + * usually one or two pageblocks anyway, so don't bother.
> + *
> + * Note that this only applies to page isolation, which calls
> + * this on random blocks in the pfn range! When we move stuff
> + * from inside the page allocator, the pages are coming off
> + * the freelist (can't be tail) and multi-block pages are
> + * handled directly in the stealing code (can't be a head).
> + */
> +
> + /* We're a tail */
> + pfn = find_straddling_buddy(start_pfn);
> + if (pfn != start_pfn) {
> + struct page *free_page = pfn_to_page(pfn);
> +
> + split_free_page(free_page, buddy_order(free_page),
> + pageblock_nr_pages, old_mt, new_mt);
> + return pageblock_nr_pages;
> + }
> +
> + /* We're a head */
> + if (PageBuddy(start_page) && buddy_order(start_page) > pageblock_order) {
> + split_free_page(start_page, buddy_order(start_page),
> + pageblock_nr_pages, new_mt, old_mt);
> + return pageblock_nr_pages;
> + }

i.e. here ^: set the mt of the block that's in isolation range, then
split the block.

I think I can guess the warning you were getting: in the head case, we
need to change the type of the head pageblock that's on the
freelist. If we do it before calling split, the
del_page_from_freelist() in there warns about the wrong type.

How about pulling the freelist removal out of split_free_page()?

del_page_from_freelist(huge_buddy);
set_pageblock_migratetype(start_page, MIGRATE_ISOLATE);
split_free_page(huge_buddy, buddy_order(), pageblock_nr_pages);
return pageblock_nr_pages;

2023-10-16 15:01:24

On 16 Oct 2023, at 14:51, Johannes Weiner wrote:

> On Mon, Oct 16, 2023 at 11:00:33AM -0400, Zi Yan wrote:
>> On 16 Oct 2023, at 10:37, Johannes Weiner wrote:
>>
>>> On Mon, Oct 16, 2023 at 09:35:34AM -0400, Zi Yan wrote:
>>>>> The attached patch has all the suggested changes, let me know how it
>>>>> looks to you. Thanks.
>>>>
>>>> The one I sent has free page accounting issues. The attached one fixes them.
>>>
>>> Do you still have the warnings? I wonder what went wrong.
>>
>> No warnings. But something with the code:
>>
>> 1. in your version, split_free_page() is called without changing any pageblock
>> migratetypes, then split_free_page() is just a no-op, since the page is
>> just deleted from the free list, then freed via different orders. Buddy allocator
>> will merge them back.
>
> Hm not quite.
>
> If it's the tail block of a buddy, I update its type before
> splitting. The splitting loop looks up the type of each block for
> sorting it onto freelists.
>
> If it's the head block, yes I split it first according to its old
> type. But then I let it fall through to scanning the block, which will
> find that buddy, update its type and move it.

That is the issue, since split_free_page() assumes the pageblocks of
that free page have different types. It basically just free the page
with different small orders summed up to the original free page order.
If all pageblocks of the free page have the same migratetype, __free_one_page()
will merge these small order pages back to the original order free page.

>
>> 2. in my version, I set pageblock migratetype to new_mt before split_free_page(),
>> but it causes free page accounting issues, since in the case of head, free pages
>> are deleted from new_mt when they are in old_mt free list and the accounting
>> decreases new_mt free page number instead of old_mt one.
>
> Right, that makes sense.
>
>> Basically, split_free_page() is awkward as it relies on preset migratetypes,
>> which changes migratetypes without deleting the free pages from the list first.
>> That is why I came up with the new split_free_page() below.
>
> Yeah, the in-between thing is bad. Either it fixes the migratetype
> before deletion, or it doesn't do the deletion. I'm thinking it would
> be simpler to move the deletion out instead.

Yes and no. After deletion, a free page no longer has PageBuddy set and
has buddy_order information cleared. Either we reset PageBuddy and order
to the deleted free page, or split_free_page() needs to be changed to
accept pages without the information (basically remove the PageBuddy
and order check code).

>>>> @@ -883,6 +886,10 @@ int split_free_page(struct page *free_page,
>>>> mt = get_pfnblock_migratetype(free_page, free_page_pfn);
>>>> del_page_from_free_list(free_page, zone, order, mt);
>>>>
>>>> + set_pageblock_migratetype(free_page, mt1);
>>>> + set_pageblock_migratetype(pfn_to_page(free_page_pfn + split_pfn_offset),
>>>> + mt2);
>>>> +
>>>> for (pfn = free_page_pfn;
>>>> pfn < free_page_pfn + (1UL << order);) {
>>>> int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);
>>>
>>> I don't think this is quite right.
>>>
>>> With CONFIG_ARCH_FORCE_MAX_ORDER it's possible that we're dealing with
>>> a buddy that is more than two blocks:
>>>
>>> [pageblock 0][pageblock 1][pageblock 2][pageblock 3]
>>> [buddy ]
>>> [isolate range ..
>>>
>>> That for loop splits the buddy into 4 blocks. The code above would set
>>> pageblock 0 to old_mt, and pageblock 1 to new_mt. But it should only
>>> set pageblock 3 to new_mt.
>>
>> OK. I think I need to fix split_free_page().
>>
>> Hmm, if CONFIG_ARCH_FORCE_MAX_ORDER can make a buddy have more than one
>> pageblock and in turn makes an in-use page have more than one pageblock,
>> we will have problems. Since in isolate_single_pageblock(), an in-use page
>> can have part of its pageblock set to a different migratetype and be freed,
>> causing the free page with unmatched migratetypes. We might need to
>> free pages at pageblock_order if their orders are bigger than pageblock_order.
>
> Is this a practical issue? You mentioned that right now only gigantic
> pages can be larger than a pageblock, and those are freed in order-0
> chunks.

Only if the system allocates a page (non hugetlb pages) with >pageblock_order
and frees it with the same order. I just do not know if such pages exist on
other arch than x86. Maybe I just think too much.

>
>>> How about pulling the freelist removal out of split_free_page()?
>>>
>>> del_page_from_freelist(huge_buddy);
>>> set_pageblock_migratetype(start_page, MIGRATE_ISOLATE);
>>> split_free_page(huge_buddy, buddy_order(), pageblock_nr_pages);
>>> return pageblock_nr_pages;
>>
>> Yes, this is better. Let me change to this implementation.
>>
>> But I would like to test it on an environment where a buddy contains more than
>> one pageblocks first. I probably can change MAX_ORDER of x86_64 to do it locally.
>> I will report back.
>
> I tweaked my version some more based on our discussion. Would you mind
> taking a look? It survived an hour of stressing with a kernel build
> and Mike's reproducer that allocates gigantics and demotes them.
>
> Note that it applies *before* consolidating of the free counts, as
> isolation needs to be fixed before the warnings are added, to avoid
> bisectability issues. The consolidation patch doesn't change it much,
> except removing freepage accounting in move_freepages_block_isolate().
>
> ---
>
> From a0460ad30a24cf73816ac40b262af0ba3723a242 Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <[email protected]>
> Date: Mon, 16 Oct 2023 12:32:21 -0400
> Subject: [PATCH] mm: page_isolation: prepare for hygienic freelists
>
> Signed-off-by: Johannes Weiner <[email protected]>
> ---
> include/linux/page-isolation.h | 4 +-
> mm/internal.h | 4 -
> mm/page_alloc.c | 198 +++++++++++++++++++--------------
> mm/page_isolation.c | 96 +++++-----------
> 4 files changed, 142 insertions(+), 160 deletions(-)
>
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index 8550b3c91480..c16db0067090 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -34,7 +34,9 @@ static inline bool is_migrate_isolate(int migratetype)
> #define REPORT_FAILURE 0x2
>
> void set_pageblock_migratetype(struct page *page, int migratetype);
> -int move_freepages_block(struct zone *zone, struct page *page, int migratetype);
> +
> +bool move_freepages_block_isolate(struct zone *zone, struct page *page,
> + int migratetype);
>
> int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
> int migratetype, int flags, gfp_t gfp_flags);
> diff --git a/mm/internal.h b/mm/internal.h
> index 3a72975425bb..0681094ad260 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -464,10 +464,6 @@ extern void *memmap_alloc(phys_addr_t size, phys_addr_t align,
> void memmap_init_range(unsigned long, int, unsigned long, unsigned long,
> unsigned long, enum meminit_context, struct vmem_altmap *, int);
>
> -
> -int split_free_page(struct page *free_page,
> - unsigned int order, unsigned long split_pfn_offset);
> -
> #if defined CONFIG_COMPACTION || defined CONFIG_CMA
>
> /*
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6185b076cf90..17e9a06027c8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -834,64 +834,6 @@ static inline void __free_one_page(struct page *page,
> page_reporting_notify_free(order);
> }
>
> -/**
> - * split_free_page() -- split a free page at split_pfn_offset
> - * @free_page: the original free page
> - * @order: the order of the page
> - * @split_pfn_offset: split offset within the page
> - *
> - * Return -ENOENT if the free page is changed, otherwise 0
> - *
> - * It is used when the free page crosses two pageblocks with different migratetypes
> - * at split_pfn_offset within the page. The split free page will be put into
> - * separate migratetype lists afterwards. Otherwise, the function achieves
> - * nothing.
> - */
> -int split_free_page(struct page *free_page,
> - unsigned int order, unsigned long split_pfn_offset)
> -{
> - struct zone *zone = page_zone(free_page);
> - unsigned long free_page_pfn = page_to_pfn(free_page);
> - unsigned long pfn;
> - unsigned long flags;
> - int free_page_order;
> - int mt;
> - int ret = 0;
> -
> - if (split_pfn_offset == 0)
> - return ret;
> -
> - spin_lock_irqsave(&zone->lock, flags);
> -
> - if (!PageBuddy(free_page) || buddy_order(free_page) != order) {
> - ret = -ENOENT;
> - goto out;
> - }
> -
> - mt = get_pfnblock_migratetype(free_page, free_page_pfn);
> - if (likely(!is_migrate_isolate(mt)))
> - __mod_zone_freepage_state(zone, -(1UL << order), mt);
> -
> - del_page_from_free_list(free_page, zone, order);
> - for (pfn = free_page_pfn;
> - pfn < free_page_pfn + (1UL << order);) {
> - int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);
> -
> - free_page_order = min_t(unsigned int,
> - pfn ? __ffs(pfn) : order,
> - __fls(split_pfn_offset));
> - __free_one_page(pfn_to_page(pfn), pfn, zone, free_page_order,
> - mt, FPI_NONE);
> - pfn += 1UL << free_page_order;
> - split_pfn_offset -= (1UL << free_page_order);
> - /* we have done the first part, now switch to second part */
> - if (split_pfn_offset == 0)
> - split_pfn_offset = (1UL << order) - (pfn - free_page_pfn);
> - }
> -out:
> - spin_unlock_irqrestore(&zone->lock, flags);
> - return ret;
> -}
> /*
> * A bad page could be due to a number of fields. Instead of multiple branches,
> * try and check multiple fields with one check. The caller must do a detailed
> @@ -1673,8 +1615,8 @@ static bool prep_move_freepages_block(struct zone *zone, struct page *page,
> return true;
> }
>
> -int move_freepages_block(struct zone *zone, struct page *page,
> - int migratetype)
> +static int move_freepages_block(struct zone *zone, struct page *page,
> + int migratetype)
> {
> unsigned long start_pfn, end_pfn;
>
> @@ -1685,6 +1627,117 @@ int move_freepages_block(struct zone *zone, struct page *page,
> return move_freepages(zone, start_pfn, end_pfn, migratetype);
> }
>
> +#ifdef CONFIG_MEMORY_ISOLATION
> +/* Look for a multi-block buddy that straddles start_pfn */
> +static unsigned long find_large_buddy(unsigned long start_pfn)
> +{
> + int order = 0;
> + struct page *page;
> + unsigned long pfn = start_pfn;
> +
> + while (!PageBuddy(page = pfn_to_page(pfn))) {
> + /* Nothing found */
> + if (++order > MAX_ORDER)
> + return start_pfn;
> + pfn &= ~0UL << order;
> + }
> +
> + /*
> + * Found a preceding buddy, but does it straddle?
> + */
> + if (pfn + (1 << buddy_order(page)) > start_pfn)
> + return pfn;
> +
> + /* Nothing found */
> + return start_pfn;
> +}
> +
> +/* Split a multi-block buddy into its individual pageblocks */
> +static void split_large_buddy(struct page *buddy, int order)
> +{
> + unsigned long pfn = page_to_pfn(buddy);
> + unsigned long end = pfn + (1 << order);
> + struct zone *zone = page_zone(buddy);
> +
> + lockdep_assert_held(&zone->lock);
> + VM_WARN_ON_ONCE(PageBuddy(buddy));
> +
> + while (pfn < end) {
> + int mt = get_pfnblock_migratetype(buddy, pfn);
> +
> + __free_one_page(buddy, pfn, zone, pageblock_order, mt, FPI_NONE);
> + pfn += pageblock_nr_pages;
> + buddy = pfn_to_page(pfn);
> + }
> +}
> +
> +/**
> + * move_freepages_block_isolate - move free pages in block for page isolation
> + * @zone: the zone
> + * @page: the pageblock page
> + * @migratetype: migratetype to set on the pageblock
> + *
> + * This is similar to move_freepages_block(), but handles the special
> + * case encountered in page isolation, where the block of interest
> + * might be part of a larger buddy spanning multiple pageblocks.
> + *
> + * Unlike the regular page allocator path, which moves pages while
> + * stealing buddies off the freelist, page isolation is interested in
> + * arbitrary pfn ranges that may have overlapping buddies on both ends.
> + *
> + * This function handles that. Straddling buddies are split into
> + * individual pageblocks. Only the block of interest is moved.
> + *
> + * Returns %true if pages could be moved, %false otherwise.
> + */
> +bool move_freepages_block_isolate(struct zone *zone, struct page *page,
> + int migratetype)
> +{
> + unsigned long start_pfn, end_pfn, pfn;
> + int nr_moved, mt;
> +
> + if (!prep_move_freepages_block(zone, page, &start_pfn, &end_pfn,
> + NULL, NULL))
> + return false;
> +
> + /* We're a tail block in a larger buddy */
> + pfn = find_large_buddy(start_pfn);
> + if (pfn != start_pfn) {
> + struct page *buddy = pfn_to_page(pfn);
> + int order = buddy_order(buddy);
> + int mt = get_pfnblock_migratetype(buddy, pfn);
> +
> + if (!is_migrate_isolate(mt))
> + __mod_zone_freepage_state(zone, -(1UL << order), mt);
> + del_page_from_free_list(buddy, zone, order);
> + set_pageblock_migratetype(pfn_to_page(start_pfn), migratetype);
> + split_large_buddy(buddy, order);
> + return true;
> + }
> +
> + /* We're the starting block of a larger buddy */
> + if (PageBuddy(page) && buddy_order(page) > pageblock_order) {
> + int mt = get_pfnblock_migratetype(page, pfn);
> + int order = buddy_order(page);
> +
> + if (!is_migrate_isolate(mt))
> + __mod_zone_freepage_state(zone, -(1UL << order), mt);
> + del_page_from_free_list(page, zone, order);
> + set_pageblock_migratetype(page, migratetype);
> + split_large_buddy(page, order);
> + return true;
> + }
> +
> + mt = get_pfnblock_migratetype(page, start_pfn);
> + nr_moved = move_freepages(zone, start_pfn, end_pfn, migratetype);
> + if (!is_migrate_isolate(mt))
> + __mod_zone_freepage_state(zone, -nr_moved, mt);
> + else if (!is_migrate_isolate(migratetype))
> + __mod_zone_freepage_state(zone, nr_moved, migratetype);
> + return true;
> +}
> +#endif /* CONFIG_MEMORY_ISOLATION */
> +
> static void change_pageblock_range(struct page *pageblock_page,
> int start_order, int migratetype)
> {
> @@ -6318,7 +6371,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 = {
> @@ -6391,29 +6443,7 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * We don't have to hold zone->lock here because the pages are
> * isolated thus they won't get removed from buddy.
> */
> -
> - order = 0;
> - outer_start = start;
> - while (!PageBuddy(pfn_to_page(outer_start))) {
> - if (++order > MAX_ORDER) {
> - outer_start = start;
> - break;
> - }
> - outer_start &= ~0UL << order;
> - }
> -
> - 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 = find_large_buddy(start);
>
> /* Make sure the range is really isolated. */
> if (test_pages_isolated(outer_start, end, 0)) {
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 27ee994a57d3..b4d53545496d 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -178,16 +178,10 @@ static int set_migratetype_isolate(struct page *page, int migratetype, int isol_
> unmovable = has_unmovable_pages(check_unmovable_start, check_unmovable_end,
> migratetype, isol_flags);
> if (!unmovable) {
> - int nr_pages;
> - int mt = get_pageblock_migratetype(page);
> -
> - nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE);
> - /* Block spans zone boundaries? */
> - if (nr_pages == -1) {
> + if (!move_freepages_block_isolate(zone, page, MIGRATE_ISOLATE)) {
> spin_unlock_irqrestore(&zone->lock, flags);
> return -EBUSY;
> }
> - __mod_zone_freepage_state(zone, -nr_pages, mt);
> zone->nr_isolate_pageblock++;
> spin_unlock_irqrestore(&zone->lock, flags);
> return 0;
> @@ -254,13 +248,11 @@ static void unset_migratetype_isolate(struct page *page, int migratetype)
> * allocation.
> */
> if (!isolated_page) {
> - int nr_pages = move_freepages_block(zone, page, migratetype);
> /*
> * Isolating this block already succeeded, so this
> * should not fail on zone boundaries.
> */
> - WARN_ON_ONCE(nr_pages == -1);
> - __mod_zone_freepage_state(zone, nr_pages, migratetype);
> + WARN_ON_ONCE(!move_freepages_block_isolate(zone, page, migratetype));
> } else {
> set_pageblock_migratetype(page, migratetype);
> __putback_isolated_page(page, order, migratetype);
> @@ -373,26 +365,29 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>
> VM_BUG_ON(!page);
> pfn = page_to_pfn(page);
> - /*
> - * start_pfn is MAX_ORDER_NR_PAGES aligned, if there is any
> - * free pages in [start_pfn, boundary_pfn), its head page will
> - * always be in the range.
> - */
> +
> if (PageBuddy(page)) {
> int order = buddy_order(page);
>
> - if (pfn + (1UL << order) > boundary_pfn) {
> - /* free page changed before split, check it again */
> - if (split_free_page(page, order, boundary_pfn - pfn))
> - continue;
> - }
> + /* move_freepages_block_isolate() handled this */
> + VM_WARN_ON_ONCE(pfn + (1 << order) > boundary_pfn);
>
> pfn += 1UL << order;
> continue;
> }
> +
> /*
> - * migrate compound pages then let the free page handling code
> - * above do the rest. If migration is not possible, just fail.
> + * If a compound page is straddling our block, attempt
> + * to migrate it out of the way.
> + *
> + * We don't have to worry about this creating a large
> + * free page that straddles into our block: gigantic
> + * pages are freed as order-0 chunks, and LRU pages
> + * (currently) do not exceed pageblock_order.
> + *
> + * The block of interest has already been marked
> + * MIGRATE_ISOLATE above, so when migration is done it
> + * will free its pages onto the correct freelists.
> */
> if (PageCompound(page)) {
> struct page *head = compound_head(page);
> @@ -403,16 +398,15 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
> pfn = head_pfn + nr_pages;
> continue;
> }
> +
> + VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
> +
> #if defined CONFIG_COMPACTION || defined CONFIG_CMA
> /*
> - * hugetlb, lru compound (THP), and movable compound pages
> - * can be migrated. Otherwise, fail the isolation.
> + * hugetlb, and movable compound pages can be
> + * migrated. Otherwise, fail the isolation.
> */
> - if (PageHuge(page) || PageLRU(page) || __PageMovable(page)) {
> - int order;
> - unsigned long outer_pfn;
> - int page_mt = get_pageblock_migratetype(page);
> - bool isolate_page = !is_migrate_isolate_page(page);
> + if (PageHuge(page) || __PageMovable(page)) {
> struct compact_control cc = {
> .nr_migratepages = 0,
> .order = -1,
> @@ -425,52 +419,12 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
> };
> INIT_LIST_HEAD(&cc.migratepages);
>
> - /*
> - * XXX: mark the page as MIGRATE_ISOLATE so that
> - * no one else can grab the freed page after migration.
> - * Ideally, the page should be freed as two separate
> - * pages to be added into separate migratetype free
> - * lists.
> - */
> - if (isolate_page) {
> - ret = set_migratetype_isolate(page, page_mt,
> - flags, head_pfn, head_pfn + nr_pages);
> - if (ret)
> - goto failed;
> - }
> -
> ret = __alloc_contig_migrate_range(&cc, head_pfn,
> head_pfn + nr_pages);
> -
> - /*
> - * restore the page's migratetype so that it can
> - * be split into separate migratetype free lists
> - * later.
> - */
> - if (isolate_page)
> - unset_migratetype_isolate(page, page_mt);
> -
> if (ret)
> goto failed;
> - /*
> - * reset pfn to the head of the free page, so
> - * that the free page handling code above can split
> - * the free page to the right migratetype list.
> - *
> - * head_pfn is not used here as a hugetlb page order
> - * can be bigger than MAX_ORDER, but after it is
> - * freed, the free page order is not. Use pfn within
> - * the range to find the head of the free page.
> - */
> - order = 0;
> - outer_pfn = pfn;
> - while (!PageBuddy(pfn_to_page(outer_pfn))) {
> - /* stop if we cannot find the free page */
> - if (++order > MAX_ORDER)
> - goto failed;
> - outer_pfn &= ~0UL << order;
> - }
> - pfn = outer_pfn;
> +
> + pfn = head_pfn + nr_pages;
> continue;
> } else
> #endif
> --
> 2.42.0

It looks good to me. Thanks.

Reviewed-by: Zi Yan <[email protected]>

--
Best Regards,
Yan, Zi

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature

2023-10-16 20:27:05

by Johannes Weiner

[permalink] [raw]

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

On Mon, Oct 16, 2023 at 03:49:49PM -0400, Zi Yan wrote:
> On 16 Oct 2023, at 14:51, Johannes Weiner wrote:
>
> > On Mon, Oct 16, 2023 at 11:00:33AM -0400, Zi Yan wrote:
> >> On 16 Oct 2023, at 10:37, Johannes Weiner wrote:
> >>
> >>> On Mon, Oct 16, 2023 at 09:35:34AM -0400, Zi Yan wrote:
> >>>>> The attached patch has all the suggested changes, let me know how it
> >>>>> looks to you. Thanks.
> >>>>
> >>>> The one I sent has free page accounting issues. The attached one fixes them.
> >>>
> >>> Do you still have the warnings? I wonder what went wrong.
> >>
> >> No warnings. But something with the code:
> >>
> >> 1. in your version, split_free_page() is called without changing any pageblock
> >> migratetypes, then split_free_page() is just a no-op, since the page is
> >> just deleted from the free list, then freed via different orders. Buddy allocator
> >> will merge them back.
> >
> > Hm not quite.
> >
> > If it's the tail block of a buddy, I update its type before
> > splitting. The splitting loop looks up the type of each block for
> > sorting it onto freelists.
> >
> > If it's the head block, yes I split it first according to its old
> > type. But then I let it fall through to scanning the block, which will
> > find that buddy, update its type and move it.
>
> That is the issue, since split_free_page() assumes the pageblocks of
> that free page have different types. It basically just free the page
> with different small orders summed up to the original free page order.
> If all pageblocks of the free page have the same migratetype, __free_one_page()
> will merge these small order pages back to the original order free page.

duh, of course, you're right. Thanks for patiently explaining this.

> >> 2. in my version, I set pageblock migratetype to new_mt before split_free_page(),
> >> but it causes free page accounting issues, since in the case of head, free pages
> >> are deleted from new_mt when they are in old_mt free list and the accounting
> >> decreases new_mt free page number instead of old_mt one.
> >
> > Right, that makes sense.
> >
> >> Basically, split_free_page() is awkward as it relies on preset migratetypes,
> >> which changes migratetypes without deleting the free pages from the list first.
> >> That is why I came up with the new split_free_page() below.
> >
> > Yeah, the in-between thing is bad. Either it fixes the migratetype
> > before deletion, or it doesn't do the deletion. I'm thinking it would
> > be simpler to move the deletion out instead.
>
> Yes and no. After deletion, a free page no longer has PageBuddy set and
> has buddy_order information cleared. Either we reset PageBuddy and order
> to the deleted free page, or split_free_page() needs to be changed to
> accept pages without the information (basically remove the PageBuddy
> and order check code).

Good point, that requires extra care.

It's correct in the code now, but it deserves a comment, especially
because of the "buddy" naming in the new split function.

> >> Hmm, if CONFIG_ARCH_FORCE_MAX_ORDER can make a buddy have more than one
> >> pageblock and in turn makes an in-use page have more than one pageblock,
> >> we will have problems. Since in isolate_single_pageblock(), an in-use page
> >> can have part of its pageblock set to a different migratetype and be freed,
> >> causing the free page with unmatched migratetypes. We might need to
> >> free pages at pageblock_order if their orders are bigger than pageblock_order.
> >
> > Is this a practical issue? You mentioned that right now only gigantic
> > pages can be larger than a pageblock, and those are freed in order-0
> > chunks.
>
> Only if the system allocates a page (non hugetlb pages) with >pageblock_order
> and frees it with the same order. I just do not know if such pages exist on
> other arch than x86. Maybe I just think too much.

Hm, I removed LRU pages from the handling (and added the warning) but
I left in PageMovable(). The only users are z3fold, zsmalloc and
memory ballooning. AFAICS none of them can be bigger than a pageblock.
Let me remove that and add a warning for that case as well.

This way, we only attempt to migrate hugetlb, where we know the free
path - and get warnings for anything else that's larger than expected.

This seems like the safest option. On the off chance that there is a
regression, it won't jeopardize anybody's systems, while the warning
provides all the information we need to debug what's going on.

> > From a0460ad30a24cf73816ac40b262af0ba3723a242 Mon Sep 17 00:00:00 2001
> > From: Johannes Weiner <[email protected]>
> > Date: Mon, 16 Oct 2023 12:32:21 -0400
> > Subject: [PATCH] mm: page_isolation: prepare for hygienic freelists
> >
> > Signed-off-by: Johannes Weiner <[email protected]>

> It looks good to me. Thanks.
>
> Reviewed-by: Zi Yan <[email protected]>

Thank you for all your help!

2023-10-16 20:39:55

by Johannes Weiner

[permalink] [raw]

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

On Mon, Oct 16, 2023 at 04:26:30PM -0400, Johannes Weiner wrote:
> On Mon, Oct 16, 2023 at 03:49:49PM -0400, Zi Yan wrote:
> > On 16 Oct 2023, at 14:51, Johannes Weiner wrote:
> >
> > > On Mon, Oct 16, 2023 at 11:00:33AM -0400, Zi Yan wrote:
> > >> On 16 Oct 2023, at 10:37, Johannes Weiner wrote:
> > >>
> > >>> On Mon, Oct 16, 2023 at 09:35:34AM -0400, Zi Yan wrote:
> > >>>>> The attached patch has all the suggested changes, let me know how it
> > >>>>> looks to you. Thanks.
> > >>>>
> > >>>> The one I sent has free page accounting issues. The attached one fixes them.
> > >>>
> > >>> Do you still have the warnings? I wonder what went wrong.
> > >>
> > >> No warnings. But something with the code:
> > >>
> > >> 1. in your version, split_free_page() is called without changing any pageblock
> > >> migratetypes, then split_free_page() is just a no-op, since the page is
> > >> just deleted from the free list, then freed via different orders. Buddy allocator
> > >> will merge them back.
> > >
> > > Hm not quite.
> > >
> > > If it's the tail block of a buddy, I update its type before
> > > splitting. The splitting loop looks up the type of each block for
> > > sorting it onto freelists.
> > >
> > > If it's the head block, yes I split it first according to its old
> > > type. But then I let it fall through to scanning the block, which will
> > > find that buddy, update its type and move it.
> >
> > That is the issue, since split_free_page() assumes the pageblocks of
> > that free page have different types. It basically just free the page
> > with different small orders summed up to the original free page order.
> > If all pageblocks of the free page have the same migratetype, __free_one_page()
> > will merge these small order pages back to the original order free page.
>
> duh, of course, you're right. Thanks for patiently explaining this.
>
> > >> 2. in my version, I set pageblock migratetype to new_mt before split_free_page(),
> > >> but it causes free page accounting issues, since in the case of head, free pages
> > >> are deleted from new_mt when they are in old_mt free list and the accounting
> > >> decreases new_mt free page number instead of old_mt one.
> > >
> > > Right, that makes sense.
> > >
> > >> Basically, split_free_page() is awkward as it relies on preset migratetypes,
> > >> which changes migratetypes without deleting the free pages from the list first.
> > >> That is why I came up with the new split_free_page() below.
> > >
> > > Yeah, the in-between thing is bad. Either it fixes the migratetype
> > > before deletion, or it doesn't do the deletion. I'm thinking it would
> > > be simpler to move the deletion out instead.
> >
> > Yes and no. After deletion, a free page no longer has PageBuddy set and
> > has buddy_order information cleared. Either we reset PageBuddy and order
> > to the deleted free page, or split_free_page() needs to be changed to
> > accept pages without the information (basically remove the PageBuddy
> > and order check code).
>
> Good point, that requires extra care.
>
> It's correct in the code now, but it deserves a comment, especially
> because of the "buddy" naming in the new split function.
>
> > >> Hmm, if CONFIG_ARCH_FORCE_MAX_ORDER can make a buddy have more than one
> > >> pageblock and in turn makes an in-use page have more than one pageblock,
> > >> we will have problems. Since in isolate_single_pageblock(), an in-use page
> > >> can have part of its pageblock set to a different migratetype and be freed,
> > >> causing the free page with unmatched migratetypes. We might need to
> > >> free pages at pageblock_order if their orders are bigger than pageblock_order.
> > >
> > > Is this a practical issue? You mentioned that right now only gigantic
> > > pages can be larger than a pageblock, and those are freed in order-0
> > > chunks.
> >
> > Only if the system allocates a page (non hugetlb pages) with >pageblock_order
> > and frees it with the same order. I just do not know if such pages exist on
> > other arch than x86. Maybe I just think too much.
>
> Hm, I removed LRU pages from the handling (and added the warning) but
> I left in PageMovable(). The only users are z3fold, zsmalloc and
> memory ballooning. AFAICS none of them can be bigger than a pageblock.
> Let me remove that and add a warning for that case as well.
>
> This way, we only attempt to migrate hugetlb, where we know the free
> path - and get warnings for anything else that's larger than expected.
>
> This seems like the safest option. On the off chance that there is a
> regression, it won't jeopardize anybody's systems, while the warning
> provides all the information we need to debug what's going on.

This delta on top?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b5292ad9860c..0da7c61af37e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1628,7 +1628,7 @@ static int move_freepages_block(struct zone *zone, struct page *page,
}

#ifdef CONFIG_MEMORY_ISOLATION
-/* Look for a multi-block buddy that straddles start_pfn */
+/* Look for a buddy that straddles start_pfn */
static unsigned long find_large_buddy(unsigned long start_pfn)
{
int order = 0;
@@ -1652,7 +1652,7 @@ static unsigned long find_large_buddy(unsigned long start_pfn)
return start_pfn;
}

-/* Split a multi-block buddy into its individual pageblocks */
+/* Split a multi-block free page into its individual pageblocks */
static void split_large_buddy(struct zone *zone, struct page *page,
unsigned long pfn, int order)
{
@@ -1661,6 +1661,9 @@ static void split_large_buddy(struct zone *zone, struct page *page,
VM_WARN_ON_ONCE(order < pageblock_order);
VM_WARN_ON_ONCE(pfn & (pageblock_nr_pages - 1));

+ /* Caller removed page from freelist, buddy info cleared! */
+ VM_WARN_ON_ONCE(PageBuddy(page));
+
while (pfn != end_pfn) {
int mt = get_pfnblock_migratetype(page, pfn);

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index b4d53545496d..c8b3c0699683 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -399,14 +399,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
continue;
}

- VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
-
#if defined CONFIG_COMPACTION || defined CONFIG_CMA
- /*
- * hugetlb, and movable compound pages can be
- * migrated. Otherwise, fail the isolation.
- */
- if (PageHuge(page) || __PageMovable(page)) {
+ if (PageHuge(page)) {
struct compact_control cc = {
.nr_migratepages = 0,
.order = -1,
@@ -426,9 +420,19 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,

pfn = head_pfn + nr_pages;
continue;
- } else
+ }
+
+ /*
+ * These pages are movable too, but they're
+ * not expected to exceed pageblock_order.
+ *
+ * Let us know when they do, so we can add
+ * proper free and split handling for them.
+ */
+ VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
+ VM_WARN_ON_ONCE_PAGE(__PageMovable(page), page);
#endif
- goto failed;
+ goto failed;
}

pfn++;

2023-10-16 20:49:19

by Zi Yan

[permalink] [raw]

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

On 16 Oct 2023, at 16:39, Johannes Weiner wrote:

> On Mon, Oct 16, 2023 at 04:26:30PM -0400, Johannes Weiner wrote:
>> On Mon, Oct 16, 2023 at 03:49:49PM -0400, Zi Yan wrote:
>>> On 16 Oct 2023, at 14:51, Johannes Weiner wrote:
>>>
>>>> On Mon, Oct 16, 2023 at 11:00:33AM -0400, Zi Yan wrote:
>>>>> On 16 Oct 2023, at 10:37, Johannes Weiner wrote:
>>>>>
>>>>>> On Mon, Oct 16, 2023 at 09:35:34AM -0400, Zi Yan wrote:
>>>>>>>> The attached patch has all the suggested changes, let me know how it
>>>>>>>> looks to you. Thanks.
>>>>>>>
>>>>>>> The one I sent has free page accounting issues. The attached one fixes them.
>>>>>>
>>>>>> Do you still have the warnings? I wonder what went wrong.
>>>>>
>>>>> No warnings. But something with the code:
>>>>>
>>>>> 1. in your version, split_free_page() is called without changing any pageblock
>>>>> migratetypes, then split_free_page() is just a no-op, since the page is
>>>>> just deleted from the free list, then freed via different orders. Buddy allocator
>>>>> will merge them back.
>>>>
>>>> Hm not quite.
>>>>
>>>> If it's the tail block of a buddy, I update its type before
>>>> splitting. The splitting loop looks up the type of each block for
>>>> sorting it onto freelists.
>>>>
>>>> If it's the head block, yes I split it first according to its old
>>>> type. But then I let it fall through to scanning the block, which will
>>>> find that buddy, update its type and move it.
>>>
>>> That is the issue, since split_free_page() assumes the pageblocks of
>>> that free page have different types. It basically just free the page
>>> with different small orders summed up to the original free page order.
>>> If all pageblocks of the free page have the same migratetype, __free_one_page()
>>> will merge these small order pages back to the original order free page.
>>
>> duh, of course, you're right. Thanks for patiently explaining this.
>>
>>>>> 2. in my version, I set pageblock migratetype to new_mt before split_free_page(),
>>>>> but it causes free page accounting issues, since in the case of head, free pages
>>>>> are deleted from new_mt when they are in old_mt free list and the accounting
>>>>> decreases new_mt free page number instead of old_mt one.
>>>>
>>>> Right, that makes sense.
>>>>
>>>>> Basically, split_free_page() is awkward as it relies on preset migratetypes,
>>>>> which changes migratetypes without deleting the free pages from the list first.
>>>>> That is why I came up with the new split_free_page() below.
>>>>
>>>> Yeah, the in-between thing is bad. Either it fixes the migratetype
>>>> before deletion, or it doesn't do the deletion. I'm thinking it would
>>>> be simpler to move the deletion out instead.
>>>
>>> Yes and no. After deletion, a free page no longer has PageBuddy set and
>>> has buddy_order information cleared. Either we reset PageBuddy and order
>>> to the deleted free page, or split_free_page() needs to be changed to
>>> accept pages without the information (basically remove the PageBuddy
>>> and order check code).
>>
>> Good point, that requires extra care.
>>
>> It's correct in the code now, but it deserves a comment, especially
>> because of the "buddy" naming in the new split function.
>>
>>>>> Hmm, if CONFIG_ARCH_FORCE_MAX_ORDER can make a buddy have more than one
>>>>> pageblock and in turn makes an in-use page have more than one pageblock,
>>>>> we will have problems. Since in isolate_single_pageblock(), an in-use page
>>>>> can have part of its pageblock set to a different migratetype and be freed,
>>>>> causing the free page with unmatched migratetypes. We might need to
>>>>> free pages at pageblock_order if their orders are bigger than pageblock_order.
>>>>
>>>> Is this a practical issue? You mentioned that right now only gigantic
>>>> pages can be larger than a pageblock, and those are freed in order-0
>>>> chunks.
>>>
>>> Only if the system allocates a page (non hugetlb pages) with >pageblock_order
>>> and frees it with the same order. I just do not know if such pages exist on
>>> other arch than x86. Maybe I just think too much.
>>
>> Hm, I removed LRU pages from the handling (and added the warning) but
>> I left in PageMovable(). The only users are z3fold, zsmalloc and
>> memory ballooning. AFAICS none of them can be bigger than a pageblock.
>> Let me remove that and add a warning for that case as well.
>>
>> This way, we only attempt to migrate hugetlb, where we know the free
>> path - and get warnings for anything else that's larger than expected.
>>
>> This seems like the safest option. On the off chance that there is a
>> regression, it won't jeopardize anybody's systems, while the warning
>> provides all the information we need to debug what's going on.
>
> This delta on top?
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b5292ad9860c..0da7c61af37e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1628,7 +1628,7 @@ static int move_freepages_block(struct zone *zone, struct page *page,
> }
>
> #ifdef CONFIG_MEMORY_ISOLATION
> -/* Look for a multi-block buddy that straddles start_pfn */
> +/* Look for a buddy that straddles start_pfn */
> static unsigned long find_large_buddy(unsigned long start_pfn)
> {
> int order = 0;
> @@ -1652,7 +1652,7 @@ static unsigned long find_large_buddy(unsigned long start_pfn)
> return start_pfn;
> }
>
> -/* Split a multi-block buddy into its individual pageblocks */
> +/* Split a multi-block free page into its individual pageblocks */
> static void split_large_buddy(struct zone *zone, struct page *page,
> unsigned long pfn, int order)
> {
> @@ -1661,6 +1661,9 @@ static void split_large_buddy(struct zone *zone, struct page *page,
> VM_WARN_ON_ONCE(order < pageblock_order);
> VM_WARN_ON_ONCE(pfn & (pageblock_nr_pages - 1));
>
> + /* Caller removed page from freelist, buddy info cleared! */
> + VM_WARN_ON_ONCE(PageBuddy(page));
> +
> while (pfn != end_pfn) {
> int mt = get_pfnblock_migratetype(page, pfn);
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index b4d53545496d..c8b3c0699683 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -399,14 +399,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
> continue;
> }
>
> - VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
> -
> #if defined CONFIG_COMPACTION || defined CONFIG_CMA
> - /*
> - * hugetlb, and movable compound pages can be
> - * migrated. Otherwise, fail the isolation.
> - */
> - if (PageHuge(page) || __PageMovable(page)) {
> + if (PageHuge(page)) {
> struct compact_control cc = {
> .nr_migratepages = 0,
> .order = -1,
> @@ -426,9 +420,19 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>
> pfn = head_pfn + nr_pages;
> continue;
> - } else
> + }
> +
> + /*
> + * These pages are movable too, but they're
> + * not expected to exceed pageblock_order.
> + *
> + * Let us know when they do, so we can add
> + * proper free and split handling for them.
> + */
> + VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
> + VM_WARN_ON_ONCE_PAGE(__PageMovable(page), page);
> #endif
> - goto failed;
> + goto failed;
> }
>
> pfn++;

LGTM.

I was thinking about adding

VM_WARN_ON_ONCE(order > pageblock_order, page);

in __free_pages() to catch all possible cases, but that is a really hot path.

And just for the record, we probably can easily fix the above warnings,
if they ever show up, by freeing >pageblock_order pages in unit of
pageblock_order.

--
Best Regards,
Yan, Zi

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature