[PATCH 0/6] Misc cleanups and improvements for compaction

[PATCH 0/6] Misc cleanups and improvements for compaction

[Baolin Wang]

Hi,

This series cantains some cleanups and improvements for compaction.
Please help to review. Thanks.

Baolin Wang (6):
  mm: compaction: drop the redundant page validation in
    update_pageblock_skip()
  mm: compaction: change fast_isolate_freepages() to void type
  mm: compaction: skip more fully scanned pageblock
  mm: compaction: only set skip flag if cc->no_set_skip_hint is false
  mm: compaction: add trace event for fast freepages isolation
  mm: compaction: skip fast freepages isolation if enough freepages are
    isolated

 include/trace/events/compaction.h | 11 +++++++++++
 mm/compaction.c                   | 25 ++++++++++++++-----------
 2 files changed, 25 insertions(+), 11 deletions(-)

-- 
2.27.0

[PATCH 1/6] mm: compaction: drop the redundant page validation in update_pageblock_skip()

[Baolin Wang]

The caller has validated the page before calling pdate_pageblock_skip(),
thus drop the redundant page validation in update_pageblock_skip().

Signed-off-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 163e2ec70aff..426bb6ce070b 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -436,9 +436,6 @@ static void update_pageblock_skip(struct compact_control *cc,
 	if (cc->no_set_skip_hint)
 		return;
 
-	if (!page)
-		return;
-
 	set_pageblock_skip(page);
 
 	/* Update where async and sync compaction should restart */
-- 
2.27.0

Re: [PATCH 1/6] mm: compaction: drop the redundant page validation in update_pageblock_skip()

[Vlastimil Babka]

On 5/25/23 14:53, Baolin Wang wrote:
> The caller has validated the page before calling pdate_pageblock_skip(),
                                                   ^ u

> thus drop the redundant page validation in update_pageblock_skip().
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

> ---
>  mm/compaction.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 163e2ec70aff..426bb6ce070b 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -436,9 +436,6 @@ static void update_pageblock_skip(struct compact_control *cc,
>  	if (cc->no_set_skip_hint)
>  		return;
>  
> -	if (!page)
> -		return;
> -
>  	set_pageblock_skip(page);
>  
>  	/* Update where async and sync compaction should restart */

Re: [PATCH 1/6] mm: compaction: drop the redundant page validation in update_pageblock_skip()

[Baolin Wang]

On 5/30/2023 3:27 PM, Vlastimil Babka wrote:
> On 5/25/23 14:53, Baolin Wang wrote:
>> The caller has validated the page before calling pdate_pageblock_skip(),
>                                                     ^ u

Ah, sorry for typos. And I see Andrew has already helped to correct this 
typo. Thanks Andrew :)

>> thus drop the redundant page validation in update_pageblock_skip().
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks for your reviewing.

>> ---
>>   mm/compaction.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 163e2ec70aff..426bb6ce070b 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -436,9 +436,6 @@ static void update_pageblock_skip(struct compact_control *cc,
>>   	if (cc->no_set_skip_hint)
>>   		return;
>>   
>> -	if (!page)
>> -		return;
>> -
>>   	set_pageblock_skip(page);
>>   
>>   	/* Update where async and sync compaction should restart */

[PATCH 2/6] mm: compaction: change fast_isolate_freepages() to void type

[Baolin Wang]

No caller cares about the return value of fast_isolate_freepages(),
void it.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 426bb6ce070b..3737c6591bfb 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1435,8 +1435,7 @@ static int next_search_order(struct compact_control *cc, int order)
 	return order;
 }
 
-static unsigned long
-fast_isolate_freepages(struct compact_control *cc)
+static void fast_isolate_freepages(struct compact_control *cc)
 {
 	unsigned int limit = max(1U, freelist_scan_limit(cc) >> 1);
 	unsigned int nr_scanned = 0;
@@ -1449,7 +1448,7 @@ fast_isolate_freepages(struct compact_control *cc)
 
 	/* Full compaction passes in a negative order */
 	if (cc->order <= 0)
-		return cc->free_pfn;
+		return;
 
 	/*
 	 * If starting the scan, use a deeper search and use the highest
@@ -1588,11 +1587,10 @@ fast_isolate_freepages(struct compact_control *cc)
 
 	cc->total_free_scanned += nr_scanned;
 	if (!page)
-		return cc->free_pfn;
+		return;
 
 	low_pfn = page_to_pfn(page);
 	fast_isolate_around(cc, low_pfn);
-	return low_pfn;
 }
 
 /*
-- 
2.27.0

Re: [PATCH 2/6] mm: compaction: change fast_isolate_freepages() to void type

[Vlastimil Babka]

On 5/25/23 14:53, Baolin Wang wrote:
> No caller cares about the return value of fast_isolate_freepages(),
> void it.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

[PATCH 3/6] mm: compaction: skip more fully scanned pageblock

[Baolin Wang]

In fast_isolate_around(), it assumes the pageblock is fully scanned if
cc->nr_freepages < cc->nr_migratepages after trying to isolate some free
pages, and will set skip flag to avoid scanning in future. However this
can miss setting the skip flag for a fully scanned pageblock (returned
'start_pfn' is equal to 'end_pfn') in the case where cc->nr_freepages
is larger than cc->nr_migratepages.

So using the returned 'start_pfn' from isolate_freepages_block() and
'end_pfn' to decide if a pageblock is fully scanned makes more sense.
It can also cover the case where cc->nr_freepages < cc->nr_migratepages,
which means the 'start_pfn' is usually equal to 'end_pfn' except some
uncommon fatal error occurs after non-strict mode isolation.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 3737c6591bfb..1e5183f39ca9 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1411,7 +1411,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 (cc->nr_freepages < cc->nr_migratepages)
+	if (start_pfn == end_pfn)
 		set_pageblock_skip(page);
 
 	return;
-- 
2.27.0

Re: [PATCH 3/6] mm: compaction: skip more fully scanned pageblock

[Vlastimil Babka]

On 5/25/23 14:53, Baolin Wang wrote:
> In fast_isolate_around(), it assumes the pageblock is fully scanned if
> cc->nr_freepages < cc->nr_migratepages after trying to isolate some free
> pages, and will set skip flag to avoid scanning in future. However this
> can miss setting the skip flag for a fully scanned pageblock (returned
> 'start_pfn' is equal to 'end_pfn') in the case where cc->nr_freepages
> is larger than cc->nr_migratepages.
> 
> So using the returned 'start_pfn' from isolate_freepages_block() and
> 'end_pfn' to decide if a pageblock is fully scanned makes more sense.
> It can also cover the case where cc->nr_freepages < cc->nr_migratepages,
> which means the 'start_pfn' is usually equal to 'end_pfn' except some
> uncommon fatal error occurs after non-strict mode isolation.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

> ---
>  mm/compaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 3737c6591bfb..1e5183f39ca9 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1411,7 +1411,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 (cc->nr_freepages < cc->nr_migratepages)
> +	if (start_pfn == end_pfn)
>  		set_pageblock_skip(page);
>  
>  	return;

[PATCH 4/6] mm: compaction: only set skip flag if cc->no_set_skip_hint is false

[Baolin Wang]

To keep the same logic as test_and_set_skip(), only set the skip flag
if cc->no_set_skip_hint is false, which makes code more reasonable.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 1e5183f39ca9..65d8d9223acc 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1223,7 +1223,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 	 * rescanned twice in a row.
 	 */
 	if (low_pfn == end_pfn && (!nr_isolated || cc->finish_pageblock)) {
-		if (valid_page && !skip_updated)
+		if (!cc->no_set_skip_hint && valid_page && !skip_updated)
 			set_pageblock_skip(valid_page);
 		update_cached_migrate(cc, low_pfn);
 	}
-- 
2.27.0

Re: [PATCH 4/6] mm: compaction: only set skip flag if cc->no_set_skip_hint is false

[Vlastimil Babka]

On 5/25/23 14:53, Baolin Wang wrote:
> To keep the same logic as test_and_set_skip(), only set the skip flag
> if cc->no_set_skip_hint is false, which makes code more reasonable.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

> ---
>  mm/compaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1e5183f39ca9..65d8d9223acc 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1223,7 +1223,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  	 * rescanned twice in a row.
>  	 */
>  	if (low_pfn == end_pfn && (!nr_isolated || cc->finish_pageblock)) {
> -		if (valid_page && !skip_updated)
> +		if (!cc->no_set_skip_hint && valid_page && !skip_updated)
>  			set_pageblock_skip(valid_page);
>  		update_cached_migrate(cc, low_pfn);
>  	}

[PATCH 5/6] mm: compaction: add trace event for fast freepages isolation

[Baolin Wang]

The fast_isolate_freepages() can also isolate freepages, but we can not
know the fast isolation efficiency to understand the fast isolation pressure.
So add a trace event to show some numbers to help to understand the efficiency
for fast freepages isolation.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 include/trace/events/compaction.h | 11 +++++++++++
 mm/compaction.c                   |  6 +++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/compaction.h b/include/trace/events/compaction.h
index 3313eb83c117..2b2a975efd20 100644
--- a/include/trace/events/compaction.h
+++ b/include/trace/events/compaction.h
@@ -64,6 +64,17 @@ DEFINE_EVENT(mm_compaction_isolate_template, mm_compaction_isolate_freepages,
 	TP_ARGS(start_pfn, end_pfn, nr_scanned, nr_taken)
 );
 
+DEFINE_EVENT(mm_compaction_isolate_template, mm_compaction_fast_isolate_freepages,
+
+	TP_PROTO(
+		unsigned long start_pfn,
+		unsigned long end_pfn,
+		unsigned long nr_scanned,
+		unsigned long nr_taken),
+
+	TP_ARGS(start_pfn, end_pfn, nr_scanned, nr_taken)
+);
+
 #ifdef CONFIG_COMPACTION
 TRACE_EVENT(mm_compaction_migratepages,
 
diff --git a/mm/compaction.c b/mm/compaction.c
index 65d8d9223acc..eccec84dae82 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1438,7 +1438,7 @@ static int next_search_order(struct compact_control *cc, int order)
 static void fast_isolate_freepages(struct compact_control *cc)
 {
 	unsigned int limit = max(1U, freelist_scan_limit(cc) >> 1);
-	unsigned int nr_scanned = 0;
+	unsigned int nr_scanned = 0, total_isolated = 0;
 	unsigned long low_pfn, min_pfn, highest = 0;
 	unsigned long nr_isolated = 0;
 	unsigned long distance;
@@ -1537,6 +1537,7 @@ static void fast_isolate_freepages(struct compact_control *cc)
 				set_page_private(page, order);
 				nr_isolated = 1 << order;
 				nr_scanned += nr_isolated - 1;
+				total_isolated += nr_isolated;
 				cc->nr_freepages += nr_isolated;
 				list_add_tail(&page->lru, &cc->freepages);
 				count_compact_events(COMPACTISOLATED, nr_isolated);
@@ -1557,6 +1558,9 @@ static void fast_isolate_freepages(struct compact_control *cc)
 			limit = max(1U, limit >> 1);
 	}
 
+	trace_mm_compaction_fast_isolate_freepages(min_pfn, cc->free_pfn,
+						   nr_scanned, total_isolated);
+
 	if (!page) {
 		cc->fast_search_fail++;
 		if (scan_start) {
-- 
2.27.0

Re: [PATCH 5/6] mm: compaction: add trace event for fast freepages isolation

[Vlastimil Babka]

On 5/25/23 14:54, Baolin Wang wrote:
> The fast_isolate_freepages() can also isolate freepages, but we can not
> know the fast isolation efficiency to understand the fast isolation pressure.
> So add a trace event to show some numbers to help to understand the efficiency
> for fast freepages isolation.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

[PATCH 6/6] mm: compaction: skip fast freepages isolation if enough freepages are isolated

[Baolin Wang]

I've observed that fast isolation often isolates more pages than
cc->migratepages, and the excess freepages will be released back to the
buddy system. So skip fast freepages isolation if enough freepages are
isolated to save some CPU cycles.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/compaction.c b/mm/compaction.c
index eccec84dae82..3ade4c095ed2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1550,6 +1550,10 @@ static void fast_isolate_freepages(struct compact_control *cc)
 
 		spin_unlock_irqrestore(&cc->zone->lock, flags);
 
+		/* Skip fast search if enough freepages isolated */
+		if (cc->nr_freepages >= cc->nr_migratepages)
+			break;
+
 		/*
 		 * Smaller scan on next order so the total scan is related
 		 * to freelist_scan_limit.
-- 
2.27.0

Re: [PATCH 6/6] mm: compaction: skip fast freepages isolation if enough freepages are isolated

[Vlastimil Babka]

On 5/25/23 14:54, Baolin Wang wrote:
> I've observed that fast isolation often isolates more pages than
> cc->migratepages, and the excess freepages will be released back to the
> buddy system. So skip fast freepages isolation if enough freepages are
> isolated to save some CPU cycles.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

> ---
>  mm/compaction.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index eccec84dae82..3ade4c095ed2 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1550,6 +1550,10 @@ static void fast_isolate_freepages(struct compact_control *cc)
>  
>  		spin_unlock_irqrestore(&cc->zone->lock, flags);
>  
> +		/* Skip fast search if enough freepages isolated */
> +		if (cc->nr_freepages >= cc->nr_migratepages)
> +			break;
> +
>  		/*
>  		 * Smaller scan on next order so the total scan is related
>  		 * to freelist_scan_limit.