[PATCH 0/5] Fixes and cleanups to compaction

[PATCH 0/5] Fixes and cleanups to compaction

[Kemeng Shi]

Hi all, this series contains random fixes and cleanups to free page
isolation in compaction. This is based on another compact series[1].
More details can be found in respective patches. Thanks!

[1] https://lore.kernel.org/all/20230728171037.2219226-1-shikemeng@huaweicloud.com/

Kemeng Shi (5):
  mm/compaction: allow blockpfn outside of pageblock for high order
    buddy page
  mm/compaction: set compact_cached_free_pfn correctly in
    update_pageblock_skip
  mm/compaction: merge end_pfn boundary check in isolate_freepages_range
  mm/compaction: remove unnecessary cursor page in
    isolate_freepages_block
  mm/compaction: remove unnecessary "else continue" at end of loop in
    isolate_freepages_block

 mm/compaction.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

-- 
2.30.0

[PATCH 1/5] mm/compaction: allow blockpfn outside of pageblock for high order buddy page

[Kemeng Shi]

Commit 9fcd6d2e052ee ("mm, compaction: skip compound pages by order in
free scanner") skiped compound pages to save iterations and limit blockpfn
to reach outside of page block in case of bogus compound_order. While this
also limit pfnblock outside page block in case a buddy page with order
higher than page block is found. After this, isolate_freepages_range will
fail unexpectedly as it will fail to isolate the page block which was
isolated successfully by high order buddy page in previous page block
and abort the free page isolation.

Fix this by allow blockpfn outside of pageblock in case of high order
buddy page.

Fixes: 9fcd6d2e052ee ("mm, compaction: skip compound pages by order in free scanner")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 6841c0496223..d1d28d57e5bd 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -681,8 +681,10 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 	/*
 	 * There is a tiny chance that we have read bogus compound_order(),
 	 * so be careful to not go outside of the pageblock.
+	 * Allow blockpfn outside pageblock in normal case that we isolate
+	 * buddy page with order more than pageblock order.
 	 */
-	if (unlikely(blockpfn > end_pfn))
+	if (unlikely(blockpfn > end_pfn) && total_isolated <= pageblock_nr_pages)
 		blockpfn = end_pfn;
 
 	trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn,
@@ -1418,7 +1420,7 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn)
 	isolate_freepages_block(cc, &start_pfn, end_pfn, &cc->freepages, 1, false);
 
 	/* Skip this pageblock in the future as it's full or nearly full */
-	if (start_pfn == end_pfn && !cc->no_set_skip_hint)
+	if (start_pfn >= end_pfn && !cc->no_set_skip_hint)
 		set_pageblock_skip(page);
 }
 
@@ -1687,7 +1689,7 @@ static void isolate_freepages(struct compact_control *cc)
 					block_end_pfn, freelist, stride, false);
 
 		/* Update the skip hint if the full pageblock was scanned */
-		if (isolate_start_pfn == block_end_pfn)
+		if (isolate_start_pfn >= block_end_pfn)
 			update_pageblock_skip(cc, page, block_start_pfn);
 
 		/* Are enough freepages isolated? */
-- 
2.30.0

Re: [PATCH 1/5] mm/compaction: allow blockpfn outside of pageblock for high order buddy page

[Andrew Morton]

On Sun, 30 Jul 2023 01:43:50 +0800 Kemeng Shi <shikemeng@huaweicloud.com> wrote:

> Commit 9fcd6d2e052ee ("mm, compaction: skip compound pages by order in
> free scanner") skiped compound pages to save iterations and limit blockpfn
> to reach outside of page block in case of bogus compound_order. While this
> also limit pfnblock outside page block in case a buddy page with order
> higher than page block is found. After this, isolate_freepages_range will
> fail unexpectedly as it will fail to isolate the page block which was
> isolated successfully by high order buddy page in previous page block
> and abort the free page isolation.
> 
> Fix this by allow blockpfn outside of pageblock in case of high order
> buddy page.
> 
> ...
>
> @@ -1418,7 +1420,7 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn)
>  	isolate_freepages_block(cc, &start_pfn, end_pfn, &cc->freepages, 1, false);
>  
>  	/* Skip this pageblock in the future as it's full or nearly full */
> -	if (start_pfn == end_pfn && !cc->no_set_skip_hint)
> +	if (start_pfn >= end_pfn && !cc->no_set_skip_hint)
>  		set_pageblock_skip(page);
>  }
>  

This needed alteration for mm-unstable changes:

@@ -1441,7 +1443,7 @@ fast_isolate_around(struct compact_contr
 	isolate_freepages_block(cc, &start_pfn, end_pfn, &cc->freepages, 1, false);
 
 	/* Skip this pageblock in the future as it's full or nearly full */
-	if (start_pfn == end_pfn)
+	if (start_pfn >= end_pfn)
 		set_pageblock_skip(page);
 
 	return;

Re: [PATCH 1/5] mm/compaction: allow blockpfn outside of pageblock for high order buddy page

[Baolin Wang]

On 7/30/2023 1:43 AM, Kemeng Shi wrote:
> Commit 9fcd6d2e052ee ("mm, compaction: skip compound pages by order in
> free scanner") skiped compound pages to save iterations and limit blockpfn
> to reach outside of page block in case of bogus compound_order. While this
> also limit pfnblock outside page block in case a buddy page with order
> higher than page block is found. After this, isolate_freepages_range will
> fail unexpectedly as it will fail to isolate the page block which was
> isolated successfully by high order buddy page in previous page block
> and abort the free page isolation.

Not sure I uderstand the problem, why the isolate_freepages_range() will 
fail? In isolate_freepages_range(), it did not use the 'blockpfn' cursor 
to try next candidate pfn, instead using the 'isolated' to calculate 
next cursor. Or I missed something else?

> Fix this by allow blockpfn outside of pageblock in case of high order
> buddy page.
> 
> Fixes: 9fcd6d2e052ee ("mm, compaction: skip compound pages by order in free scanner")
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>   mm/compaction.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 6841c0496223..d1d28d57e5bd 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -681,8 +681,10 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   	/*
>   	 * There is a tiny chance that we have read bogus compound_order(),
>   	 * so be careful to not go outside of the pageblock.
> +	 * Allow blockpfn outside pageblock in normal case that we isolate
> +	 * buddy page with order more than pageblock order.
>   	 */
> -	if (unlikely(blockpfn > end_pfn))
> +	if (unlikely(blockpfn > end_pfn) && total_isolated <= pageblock_nr_pages)
>   		blockpfn = end_pfn;
>   
>   	trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn,
> @@ -1418,7 +1420,7 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn)
>   	isolate_freepages_block(cc, &start_pfn, end_pfn, &cc->freepages, 1, false);
>   
>   	/* Skip this pageblock in the future as it's full or nearly full */
> -	if (start_pfn == end_pfn && !cc->no_set_skip_hint)
> +	if (start_pfn >= end_pfn && !cc->no_set_skip_hint)
>   		set_pageblock_skip(page);
>   }
>   
> @@ -1687,7 +1689,7 @@ static void isolate_freepages(struct compact_control *cc)
>   					block_end_pfn, freelist, stride, false);
>   
>   		/* Update the skip hint if the full pageblock was scanned */
> -		if (isolate_start_pfn == block_end_pfn)
> +		if (isolate_start_pfn >= block_end_pfn)
>   			update_pageblock_skip(cc, page, block_start_pfn);
>   
>   		/* Are enough freepages isolated? */

Re: [PATCH 1/5] mm/compaction: allow blockpfn outside of pageblock for high order buddy page

[Kemeng Shi]

on 8/2/2023 9:57 AM, Baolin Wang wrote:
> 
> 
> On 7/30/2023 1:43 AM, Kemeng Shi wrote:
>> Commit 9fcd6d2e052ee ("mm, compaction: skip compound pages by order in
>> free scanner") skiped compound pages to save iterations and limit blockpfn
>> to reach outside of page block in case of bogus compound_order. While this
>> also limit pfnblock outside page block in case a buddy page with order
>> higher than page block is found. After this, isolate_freepages_range will
>> fail unexpectedly as it will fail to isolate the page block which was
>> isolated successfully by high order buddy page in previous page block
>> and abort the free page isolation.
> 
> Not sure I uderstand the problem, why the isolate_freepages_range() will fail? In isolate_freepages_range(), it did not use the 'blockpfn' cursor to try next candidate pfn, instead using the 'isolated' to calculate next cursor. Or I missed something else?
> 
Ah, my bad. I will drop this in next version.

>> Fix this by allow blockpfn outside of pageblock in case of high order
>> buddy page.
>>
>> Fixes: 9fcd6d2e052ee ("mm, compaction: skip compound pages by order in free scanner")
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>   mm/compaction.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 6841c0496223..d1d28d57e5bd 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -681,8 +681,10 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>>       /*
>>        * There is a tiny chance that we have read bogus compound_order(),
>>        * so be careful to not go outside of the pageblock.
>> +     * Allow blockpfn outside pageblock in normal case that we isolate
>> +     * buddy page with order more than pageblock order.
>>        */
>> -    if (unlikely(blockpfn > end_pfn))
>> +    if (unlikely(blockpfn > end_pfn) && total_isolated <= pageblock_nr_pages)
>>           blockpfn = end_pfn;
>>         trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn,
>> @@ -1418,7 +1420,7 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn)
>>       isolate_freepages_block(cc, &start_pfn, end_pfn, &cc->freepages, 1, false);
>>         /* Skip this pageblock in the future as it's full or nearly full */
>> -    if (start_pfn == end_pfn && !cc->no_set_skip_hint)
>> +    if (start_pfn >= end_pfn && !cc->no_set_skip_hint)
>>           set_pageblock_skip(page);
>>   }
>>   @@ -1687,7 +1689,7 @@ static void isolate_freepages(struct compact_control *cc)
>>                       block_end_pfn, freelist, stride, false);
>>             /* Update the skip hint if the full pageblock was scanned */
>> -        if (isolate_start_pfn == block_end_pfn)
>> +        if (isolate_start_pfn >= block_end_pfn)
>>               update_pageblock_skip(cc, page, block_start_pfn);
>>             /* Are enough freepages isolated? */
> 

[PATCH 2/5] mm/compaction: set compact_cached_free_pfn correctly in update_pageblock_skip

[Kemeng Shi]

We will set skip to page block of block_start_pfn, it's more reasonable
to set compact_cached_free_pfn to page block before the block_start_pfn.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index d1d28d57e5bd..4a784872565a 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1690,7 +1690,8 @@ static void isolate_freepages(struct compact_control *cc)
 
 		/* Update the skip hint if the full pageblock was scanned */
 		if (isolate_start_pfn >= block_end_pfn)
-			update_pageblock_skip(cc, page, block_start_pfn);
+			update_pageblock_skip(cc, page, block_start_pfn -
+					      pageblock_nr_pages);
 
 		/* Are enough freepages isolated? */
 		if (cc->nr_freepages >= cc->nr_migratepages) {
-- 
2.30.0

Re: [PATCH 2/5] mm/compaction: set compact_cached_free_pfn correctly in update_pageblock_skip

[Baolin Wang]

On 7/30/2023 1:43 AM, Kemeng Shi wrote:
> We will set skip to page block of block_start_pfn, it's more reasonable
> to set compact_cached_free_pfn to page block before the block_start_pfn.

Looks reasonable to me.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>   mm/compaction.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index d1d28d57e5bd..4a784872565a 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1690,7 +1690,8 @@ static void isolate_freepages(struct compact_control *cc)
>   
>   		/* Update the skip hint if the full pageblock was scanned */
>   		if (isolate_start_pfn >= block_end_pfn)
> -			update_pageblock_skip(cc, page, block_start_pfn);
> +			update_pageblock_skip(cc, page, block_start_pfn -
> +					      pageblock_nr_pages);
>   
>   		/* Are enough freepages isolated? */
>   		if (cc->nr_freepages >= cc->nr_migratepages) {

[PATCH 3/5] mm/compaction: merge end_pfn boundary check in isolate_freepages_range

[Kemeng Shi]

From: Kemeng Shi <shikemeng@huawei.com>

Merge the end_pfn boundary check for single page block forward and multiple
page blocks forward to avoid do twice boundary check for multiple page
blocks forward.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 4a784872565a..65791a74c5e8 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -740,8 +740,6 @@ isolate_freepages_range(struct compact_control *cc,
 		/* Protect pfn from changing by isolate_freepages_block */
 		unsigned long isolate_start_pfn = pfn;
 
-		block_end_pfn = min(block_end_pfn, end_pfn);
-
 		/*
 		 * pfn could pass the block_end_pfn if isolated freepage
 		 * is more than pageblock order. In this case, we adjust
@@ -750,9 +748,10 @@ isolate_freepages_range(struct compact_control *cc,
 		if (pfn >= block_end_pfn) {
 			block_start_pfn = pageblock_start_pfn(pfn);
 			block_end_pfn = pageblock_end_pfn(pfn);
-			block_end_pfn = min(block_end_pfn, end_pfn);
 		}
 
+		block_end_pfn = min(block_end_pfn, end_pfn);
+
 		if (!pageblock_pfn_to_page(block_start_pfn,
 					block_end_pfn, cc->zone))
 			break;
-- 
2.30.0

Re: [PATCH 3/5] mm/compaction: merge end_pfn boundary check in isolate_freepages_range

[Baolin Wang]

On 7/30/2023 1:43 AM, Kemeng Shi wrote:
> From: Kemeng Shi <shikemeng@huawei.com>
> 
> Merge the end_pfn boundary check for single page block forward and multiple
> page blocks forward to avoid do twice boundary check for multiple page
> blocks forward.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/compaction.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 4a784872565a..65791a74c5e8 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -740,8 +740,6 @@ isolate_freepages_range(struct compact_control *cc,
>   		/* Protect pfn from changing by isolate_freepages_block */
>   		unsigned long isolate_start_pfn = pfn;
>   
> -		block_end_pfn = min(block_end_pfn, end_pfn);
> -
>   		/*
>   		 * pfn could pass the block_end_pfn if isolated freepage
>   		 * is more than pageblock order. In this case, we adjust
> @@ -750,9 +748,10 @@ isolate_freepages_range(struct compact_control *cc,
>   		if (pfn >= block_end_pfn) {
>   			block_start_pfn = pageblock_start_pfn(pfn);
>   			block_end_pfn = pageblock_end_pfn(pfn);
> -			block_end_pfn = min(block_end_pfn, end_pfn);
>   		}
>   
> +		block_end_pfn = min(block_end_pfn, end_pfn);
> +
>   		if (!pageblock_pfn_to_page(block_start_pfn,
>   					block_end_pfn, cc->zone))
>   			break;

[PATCH 4/5] mm/compaction: remove unnecessary cursor page in isolate_freepages_block

[Kemeng Shi]

The cursor is only used for page forward currently. We can simply move page
forward directly to remove unnecessary cursor.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 65791a74c5e8..cfb661f4ce23 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -589,7 +589,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 				bool strict)
 {
 	int nr_scanned = 0, total_isolated = 0;
-	struct page *cursor;
+	struct page *page;
 	unsigned long flags = 0;
 	spinlock_t *locked = NULL;
 	unsigned long blockpfn = *start_pfn;
@@ -599,12 +599,11 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 	if (strict)
 		stride = 1;
 
-	cursor = pfn_to_page(blockpfn);
+	page = pfn_to_page(blockpfn);
 
 	/* Isolate free pages. */
-	for (; blockpfn < end_pfn; blockpfn += stride, cursor += stride) {
+	for (; blockpfn < end_pfn; blockpfn += stride, page += stride) {
 		int isolated;
-		struct page *page = cursor;
 
 		/*
 		 * Periodically drop the lock (if held) regardless of its
@@ -628,7 +627,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 
 			if (likely(order <= MAX_ORDER)) {
 				blockpfn += (1UL << order) - 1;
-				cursor += (1UL << order) - 1;
+				page += (1UL << order) - 1;
 				nr_scanned += (1UL << order) - 1;
 			}
 			goto isolate_fail;
@@ -665,7 +664,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 		}
 		/* Advance to the end of split page */
 		blockpfn += isolated - 1;
-		cursor += isolated - 1;
+		page += isolated - 1;
 		continue;
 
 isolate_fail:
-- 
2.30.0

Re: [PATCH 4/5] mm/compaction: remove unnecessary cursor page in isolate_freepages_block

[Baolin Wang]

On 7/30/2023 1:43 AM, Kemeng Shi wrote:
> The cursor is only used for page forward currently. We can simply move page
> forward directly to remove unnecessary cursor.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/compaction.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 65791a74c5e8..cfb661f4ce23 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -589,7 +589,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   				bool strict)
>   {
>   	int nr_scanned = 0, total_isolated = 0;
> -	struct page *cursor;
> +	struct page *page;
>   	unsigned long flags = 0;
>   	spinlock_t *locked = NULL;
>   	unsigned long blockpfn = *start_pfn;
> @@ -599,12 +599,11 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   	if (strict)
>   		stride = 1;
>   
> -	cursor = pfn_to_page(blockpfn);
> +	page = pfn_to_page(blockpfn);
>   
>   	/* Isolate free pages. */
> -	for (; blockpfn < end_pfn; blockpfn += stride, cursor += stride) {
> +	for (; blockpfn < end_pfn; blockpfn += stride, page += stride) {
>   		int isolated;
> -		struct page *page = cursor;
>   
>   		/*
>   		 * Periodically drop the lock (if held) regardless of its
> @@ -628,7 +627,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   
>   			if (likely(order <= MAX_ORDER)) {
>   				blockpfn += (1UL << order) - 1;
> -				cursor += (1UL << order) - 1;
> +				page += (1UL << order) - 1;
>   				nr_scanned += (1UL << order) - 1;
>   			}
>   			goto isolate_fail;
> @@ -665,7 +664,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   		}
>   		/* Advance to the end of split page */
>   		blockpfn += isolated - 1;
> -		cursor += isolated - 1;
> +		page += isolated - 1;
>   		continue;
>   
>   isolate_fail:

[PATCH 5/5] mm/compaction: remove unnecessary "else continue" at end of loop in isolate_freepages_block

[Kemeng Shi]

There is no behavior change to remove "else continue" code at end of scan loop.
Just remove it to make code cleaner.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index cfb661f4ce23..d38297018077 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -670,9 +670,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 isolate_fail:
 		if (strict)
 			break;
-		else
-			continue;
-
 	}
 
 	compact_unlock_irqrestore(&locked, flags);
-- 
2.30.0

Re: [PATCH 5/5] mm/compaction: remove unnecessary "else continue" at end of loop in isolate_freepages_block

[Baolin Wang]

On 7/30/2023 1:43 AM, Kemeng Shi wrote:
> There is no behavior change to remove "else continue" code at end of scan loop.
> Just remove it to make code cleaner.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/compaction.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index cfb661f4ce23..d38297018077 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -670,9 +670,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   isolate_fail:
>   		if (strict)
>   			break;
> -		else
> -			continue;
> -
>   	}
>   
>   	compact_unlock_irqrestore(&locked, flags);