[PATCH 0/5] Some small improvements for compaction

[PATCH 0/5] Some small improvements for compaction

[Baolin Wang]

Hi,

When I did some compaction testing, I found some small rooms to improve
as well as some code cleanups. Please help to review. Thanks.

Baolin Wang (5):
  mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()
  mm: compaction: Move list validation into compact_zone()
  mm: compaction: Count the migration scaned pages events for proactive
    compaction
  mm: compaction: Add missing kcompactd wakeup trace event
  mm: compaction: Avoid fragmentation score calculation for empty zones

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

-- 
2.27.0

[PATCH 1/5] mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()

[Baolin Wang]

The compaction_suitable() will never return values other than COMPACT_SUCCESS,
COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
the redundant VM_BUG_ON() validation for the return values of compaction_suitable().

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 62a61de44658..5e6f5e35748d 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2313,9 +2313,6 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 	if (ret == COMPACT_SUCCESS || ret == COMPACT_SKIPPED)
 		return ret;
 
-	/* huh, compaction_suitable is returning something unexpected */
-	VM_BUG_ON(ret != COMPACT_CONTINUE);
-
 	/*
 	 * Clear pageblock skip if there were failures recently and compaction
 	 * is about to be retried after being deferred.
-- 
2.27.0

[PATCH 2/5] mm: compaction: Move list validation into compact_zone()

[Baolin Wang]

Move the cc.freepages and cc.migratepages list validation into compact_zone()
to remove some duplicate code.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 5e6f5e35748d..f8e8addc8664 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2488,6 +2488,9 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 
 	trace_mm_compaction_end(cc, start_pfn, end_pfn, sync, ret);
 
+	VM_BUG_ON(!list_empty(&cc->freepages));
+	VM_BUG_ON(!list_empty(&cc->migratepages));
+
 	return ret;
 }
 
@@ -2526,9 +2529,6 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
 
 	ret = compact_zone(&cc, &capc);
 
-	VM_BUG_ON(!list_empty(&cc.freepages));
-	VM_BUG_ON(!list_empty(&cc.migratepages));
-
 	/*
 	 * Make sure we hide capture control first before we read the captured
 	 * page pointer, otherwise an interrupt could free and capture a page
@@ -2659,9 +2659,6 @@ static void proactive_compact_node(pg_data_t *pgdat)
 		cc.zone = zone;
 
 		compact_zone(&cc, NULL);
-
-		VM_BUG_ON(!list_empty(&cc.freepages));
-		VM_BUG_ON(!list_empty(&cc.migratepages));
 	}
 }
 
@@ -2689,9 +2686,6 @@ static void compact_node(int nid)
 		cc.zone = zone;
 
 		compact_zone(&cc, NULL);
-
-		VM_BUG_ON(!list_empty(&cc.freepages));
-		VM_BUG_ON(!list_empty(&cc.migratepages));
 	}
 }
 
@@ -2868,9 +2862,6 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 				     cc.total_migrate_scanned);
 		count_compact_events(KCOMPACTD_FREE_SCANNED,
 				     cc.total_free_scanned);
-
-		VM_BUG_ON(!list_empty(&cc.freepages));
-		VM_BUG_ON(!list_empty(&cc.migratepages));
 	}
 
 	/*
-- 
2.27.0

[PATCH 3/5] mm: compaction: Count the migration scanned pages events for proactive compaction

[Baolin Wang]

The proactive compaction will reuse per-node kcompactd threads, so we
should also count the KCOMPACTD_MIGRATE_SCANNED and KCOMPACTD_FREE_SCANNED
events for proactive compaction.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index f8e8addc8664..62f6bb68c9cb 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2659,6 +2659,11 @@ static void proactive_compact_node(pg_data_t *pgdat)
 		cc.zone = zone;
 
 		compact_zone(&cc, NULL);
+
+		count_compact_events(KCOMPACTD_MIGRATE_SCANNED,
+				     cc.total_migrate_scanned);
+		count_compact_events(KCOMPACTD_FREE_SCANNED,
+				     cc.total_free_scanned);
 	}
 }
 
-- 
2.27.0

[PATCH 4/5] mm: compaction: Add missing kcompactd wakeup trace event

[Baolin Wang]

Add missing kcompactd wakeup trace event for proactive compaction, meanwhile
use order = -1 and the highest zone index of the pgdat for the kcompactd wakeup
trace event by proactive compaction.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 62f6bb68c9cb..0fd6c81a7809 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2730,6 +2730,8 @@ int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
 				continue;
 
 			pgdat->proactive_compact_trigger = true;
+			trace_mm_compaction_wakeup_kcompactd(pgdat->node_id, -1,
+							     pgdat->nr_zones - 1);
 			wake_up_interruptible(&pgdat->kcompactd_wait);
 		}
 	}
-- 
2.27.0

[PATCH 5/5] mm: compaction: Avoid fragmentation score calculation for empty zones

[Baolin Wang]

There is no need to calculate the fragmentation score for empty zones.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 0fd6c81a7809..b758b00a4885 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2025,6 +2025,8 @@ static unsigned int fragmentation_score_node(pg_data_t *pgdat)
 		struct zone *zone;
 
 		zone = &pgdat->node_zones[zoneid];
+		if (!populated_zone(zone))
+			continue;
 		score += fragmentation_score_zone_weighted(zone);
 	}
 
-- 
2.27.0

Re: [PATCH 1/5] mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()

[Matthew Wilcox]

On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> the redundant VM_BUG_ON() validation for the return values of compaction_suitable().

I don't understand why we'd remove this check.

> 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 62a61de44658..5e6f5e35748d 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2313,9 +2313,6 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>  	if (ret == COMPACT_SUCCESS || ret == COMPACT_SKIPPED)
>  		return ret;
>  
> -	/* huh, compaction_suitable is returning something unexpected */
> -	VM_BUG_ON(ret != COMPACT_CONTINUE);
> -
>  	/*
>  	 * Clear pageblock skip if there were failures recently and compaction
>  	 * is about to be retried after being deferred.
> -- 
> 2.27.0
> 
> 

Re: [PATCH 1/5] mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()

[Andrew Morton]

On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> > The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> > COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> > and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> > the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
> 
> I don't understand why we'd remove this check.

Well, just from code inspection it serves no purpose.

Such an assertion might be useful during early code development, but I
think we can consider compaction_suitable() to adequately debugged by
now?

Re: [PATCH 1/5] mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()

[Matthew Wilcox]

On Tue, Jan 10, 2023 at 03:25:32PM -0800, Andrew Morton wrote:
> On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> > > The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> > > COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> > > and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> > > the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
> > 
> > I don't understand why we'd remove this check.
> 
> Well, just from code inspection it serves no purpose.
> 
> Such an assertion might be useful during early code development, but I
> think we can consider compaction_suitable() to adequately debugged by
> now?

What if compaction_suitable() is modified to return another value?
This seems like a relatively innocuous check.

Re: [PATCH 1/5] mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()

[Baolin Wang]

On 1/11/2023 7:37 AM, Matthew Wilcox wrote:
> On Tue, Jan 10, 2023 at 03:25:32PM -0800, Andrew Morton wrote:
>> On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>>
>>> On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
>>>> The compaction_suitable() will never return values other than COMPACT_SUCCESS,
>>>> COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
>>>> and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
>>>> the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
>>>
>>> I don't understand why we'd remove this check.
>>
>> Well, just from code inspection it serves no purpose.
>>
>> Such an assertion might be useful during early code development, but I
>> think we can consider compaction_suitable() to adequately debugged by
>> now?
> 
> What if compaction_suitable() is modified to return another value?

Then this will be an expected value which should be handled by caller, 
and IMO we can not make such assumption for future to keep this 
unhelpful check.

Re: [PATCH 1/5] mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()

[Andrew Morton]

On Wed, 11 Jan 2023 14:40:20 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> 
> 
> On 1/11/2023 7:37 AM, Matthew Wilcox wrote:
> > On Tue, Jan 10, 2023 at 03:25:32PM -0800, Andrew Morton wrote:
> >> On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> >>
> >>> On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> >>>> The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> >>>> COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> >>>> and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> >>>> the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
> >>>
> >>> I don't understand why we'd remove this check.
> >>
> >> Well, just from code inspection it serves no purpose.
> >>
> >> Such an assertion might be useful during early code development, but I
> >> think we can consider compaction_suitable() to adequately debugged by
> >> now?
> > 
> > What if compaction_suitable() is modified to return another value?
> 
> Then this will be an expected value which should be handled by caller, 
> and IMO we can not make such assumption for future to keep this 
> unhelpful check.

One way of looking at this: if the assertion wasn't there and someone
sent a patch which added it, would we merge the patch?

"[patch] add check for compaction_suitable() return value"

"why"

"it might be wrong"

"it isn't"

"but we might make it wrong later"

"the same can be said of every function in the kernel"


And if we wouldn't merge a hypothetical patch which adds some code, we
shouldn't retain that code, no?