When claiming a block during compaction isolation, move any remaining
free pages to the correct freelists as well, instead of stranding them
on the wrong list. Otherwise, this encourages incompatible page mixing
down the line, and thus long-term fragmentation.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/page_alloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3db405414174..f6f658c3d394 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2548,9 +2548,12 @@ int __isolate_free_page(struct page *page, unsigned int order)
 			 * Only change normal pageblocks (i.e., they can merge
 			 * with others)
 			 */
-			if (migratetype_is_mergeable(mt))
+			if (migratetype_is_mergeable(mt)) {
 				set_pageblock_migratetype(page,
 							  MIGRATE_MOVABLE);
+				move_freepages_block(zone, page,
+						     MIGRATE_MOVABLE, NULL);
+			}
 		}
 	}
 
-- 
2.42.0

On Mon, Sep 11, 2023 at 03:41:44PM -0400, Johannes Weiner wrote:
> When claiming a block during compaction isolation, move any remaining
> free pages to the correct freelists as well, instead of stranding them
> on the wrong list. Otherwise, this encourages incompatible page mixing
> down the line, and thus long-term fragmentation.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Hmm, this is potentially expensive in some cases but it's also correct.
Given how expensive the whole path is, I doubt it's noticable and some of
this activity will be !direct_compaction anyway and relatively invisible
even if I'm not a fan of hiding overhead in kthreads. Either way;

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

On 9/11/23 21:41, Johannes Weiner wrote:
> When claiming a block during compaction isolation, move any remaining
> free pages to the correct freelists as well, instead of stranding them
> on the wrong list. Otherwise, this encourages incompatible page mixing
> down the line, and thus long-term fragmentation.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/page_alloc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3db405414174..f6f658c3d394 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2548,9 +2548,12 @@ int __isolate_free_page(struct page *page, unsigned int order)
>  			 * Only change normal pageblocks (i.e., they can merge
>  			 * with others)
>  			 */
> -			if (migratetype_is_mergeable(mt))
> +			if (migratetype_is_mergeable(mt)) {
>  				set_pageblock_migratetype(page,
>  							  MIGRATE_MOVABLE);
> +				move_freepages_block(zone, page,
> +						     MIGRATE_MOVABLE, NULL);
> +			}
>  		}
>  	}
>

On 11 Sep 2023, at 15:41, Johannes Weiner wrote:

> When claiming a block during compaction isolation, move any remaining
> free pages to the correct freelists as well, instead of stranding them
> on the wrong list. Otherwise, this encourages incompatible page mixing
> down the line, and thus long-term fragmentation.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/page_alloc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3db405414174..f6f658c3d394 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2548,9 +2548,12 @@ int __isolate_free_page(struct page *page, unsigned int order)
>  			 * Only change normal pageblocks (i.e., they can merge
>  			 * with others)
>  			 */
> -			if (migratetype_is_mergeable(mt))
> +			if (migratetype_is_mergeable(mt)) {
>  				set_pageblock_migratetype(page,
>  							  MIGRATE_MOVABLE);
> +				move_freepages_block(zone, page,
> +						     MIGRATE_MOVABLE, NULL);
> +			}
>  		}
>  	}
>
> -- 
> 2.42.0

Is this needed? And is this correct?

__isolate_free_page() removes the free page from a free list, but the added
move_freepages_block() puts the page back to another free list, making
__isolate_free_page() not do its work. OK. the for loop is going through
the pages within the pageblock, so move_freepages_block() should be used
on the rest of the pages on the pageblock.

So to make this correct, the easies change might be move
del_page_from_free_list(page, zone, order) below this code chunk.

--
Best Regards,
Yan, Zi

On Mon, Sep 11, 2023 at 04:17:07PM -0400, Zi Yan wrote:
> On 11 Sep 2023, at 15:41, Johannes Weiner wrote:
> 
> > When claiming a block during compaction isolation, move any remaining
> > free pages to the correct freelists as well, instead of stranding them
> > on the wrong list. Otherwise, this encourages incompatible page mixing
> > down the line, and thus long-term fragmentation.
> >
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > ---
> >  mm/page_alloc.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 3db405414174..f6f658c3d394 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2548,9 +2548,12 @@ int __isolate_free_page(struct page *page, unsigned int order)
> >  			 * Only change normal pageblocks (i.e., they can merge
> >  			 * with others)
> >  			 */
> > -			if (migratetype_is_mergeable(mt))
> > +			if (migratetype_is_mergeable(mt)) {
> >  				set_pageblock_migratetype(page,
> >  							  MIGRATE_MOVABLE);
> > +				move_freepages_block(zone, page,
> > +						     MIGRATE_MOVABLE, NULL);
> > +			}
> >  		}
> >  	}
> >
> > -- 
> > 2.42.0
> 
> Is this needed?

Yes, the problem is if we e.g. isolate half a block, then we'll
convert the type of the whole block but strand the half we're not
isolating. This can be a couple of hundred pages on the wrong list.

> And is this correct?
> 
> __isolate_free_page() removes the free page from a free list, but the added
> move_freepages_block() puts the page back to another free list, making
> __isolate_free_page() not do its work. OK. the for loop is going through
> the pages within the pageblock, so move_freepages_block() should be used
> on the rest of the pages on the pageblock.
> 
> So to make this correct, the easies change might be move
> del_page_from_free_list(page, zone, order) below this code chunk.

There is a del_page_from_freelist() just above this diff hunk. That
takes the page off the list and clears its PageBuddy.

move_freepages_block() will then move only the remainder of the block
that's still on the freelist with a mismatched type (move_freepages()
only moves buddies).

On 11 Sep 2023, at 16:47, Johannes Weiner wrote:

> On Mon, Sep 11, 2023 at 04:17:07PM -0400, Zi Yan wrote:
>> On 11 Sep 2023, at 15:41, Johannes Weiner wrote:
>>
>>> When claiming a block during compaction isolation, move any remaining
>>> free pages to the correct freelists as well, instead of stranding them
>>> on the wrong list. Otherwise, this encourages incompatible page mixing
>>> down the line, and thus long-term fragmentation.
>>>
>>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>>> ---
>>>  mm/page_alloc.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 3db405414174..f6f658c3d394 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -2548,9 +2548,12 @@ int __isolate_free_page(struct page *page, unsigned int order)
>>>  			 * Only change normal pageblocks (i.e., they can merge
>>>  			 * with others)
>>>  			 */
>>> -			if (migratetype_is_mergeable(mt))
>>> +			if (migratetype_is_mergeable(mt)) {
>>>  				set_pageblock_migratetype(page,
>>>  							  MIGRATE_MOVABLE);
>>> +				move_freepages_block(zone, page,
>>> +						     MIGRATE_MOVABLE, NULL);
>>> +			}
>>>  		}
>>>  	}
>>>
>>> -- 
>>> 2.42.0
>>
>> Is this needed?
>
> Yes, the problem is if we e.g. isolate half a block, then we'll
> convert the type of the whole block but strand the half we're not
> isolating. This can be a couple of hundred pages on the wrong list.
>
>> And is this correct?
>>
>> __isolate_free_page() removes the free page from a free list, but the added
>> move_freepages_block() puts the page back to another free list, making
>> __isolate_free_page() not do its work. OK. the for loop is going through
>> the pages within the pageblock, so move_freepages_block() should be used
>> on the rest of the pages on the pageblock.
>>
>> So to make this correct, the easies change might be move
>> del_page_from_free_list(page, zone, order) below this code chunk.
>
> There is a del_page_from_freelist() just above this diff hunk. That
> takes the page off the list and clears its PageBuddy.
>
> move_freepages_block() will then move only the remainder of the block
> that's still on the freelist with a mismatched type (move_freepages()
> only moves buddies).

Ah, I missed __ClearPageBuddy() in del_page_from_free_list(). Thanks.

Reviewed-by: Zi Yan <ziy@nvidia.com>


--
Best Regards,
Yan, Zi