[PATCH] mm: compaction: refactor compact_node()

[PATCH] mm: compaction: refactor compact_node()

[Kefeng Wang]

Refactor compact_node() to handle both proactive and synchronous compact
memory, which cleanups code a bit.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/compaction.c | 66 +++++++++++++++++--------------------------------
 1 file changed, 23 insertions(+), 43 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index e63a4ee7e029..f2d886a88ee1 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2884,26 +2884,17 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
 	return rc;
 }
 
-/*
- * Compact all zones within a node till each zone's fragmentation score
- * reaches within proactive compaction thresholds (as determined by the
- * proactiveness tunable).
- *
- * It is possible that the function returns before reaching score targets
- * due to various back-off conditions, such as, contention on per-node or
- * per-zone locks.
- */
-static void proactive_compact_node(pg_data_t *pgdat)
+static void compact_node(pg_data_t *pgdat, bool proactive)
 {
 	int zoneid;
 	struct zone *zone;
 	struct compact_control cc = {
 		.order = -1,
-		.mode = MIGRATE_SYNC_LIGHT,
+		.mode = proactive ? MIGRATE_SYNC_LIGHT : MIGRATE_SYNC,
 		.ignore_skip_hint = true,
 		.whole_zone = true,
 		.gfp_mask = GFP_KERNEL,
-		.proactive_compaction = true,
+		.proactive_compaction = proactive,
 	};
 
 	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
@@ -2915,41 +2906,30 @@ static void proactive_compact_node(pg_data_t *pgdat)
 
 		compact_zone(&cc, NULL);
 
-		count_compact_events(KCOMPACTD_MIGRATE_SCANNED,
-				     cc.total_migrate_scanned);
-		count_compact_events(KCOMPACTD_FREE_SCANNED,
-				     cc.total_free_scanned);
+		if (proactive) {
+			count_compact_events(KCOMPACTD_MIGRATE_SCANNED,
+					     cc.total_migrate_scanned);
+			count_compact_events(KCOMPACTD_FREE_SCANNED,
+					     cc.total_free_scanned);
+		}
 	}
 }
 
-/* Compact all zones within a node */
-static void compact_node(int nid)
+/*
+ * Compact all zones within a node till each zone's fragmentation score
+ * reaches within proactive compaction thresholds (as determined by the
+ * proactiveness tunable).
+ *
+ * It is possible that the function returns before reaching score targets
+ * due to various back-off conditions, such as, contention on per-node or
+ * per-zone locks.
+ */
+static void proactive_compact_node(pg_data_t *pgdat)
 {
-	pg_data_t *pgdat = NODE_DATA(nid);
-	int zoneid;
-	struct zone *zone;
-	struct compact_control cc = {
-		.order = -1,
-		.mode = MIGRATE_SYNC,
-		.ignore_skip_hint = true,
-		.whole_zone = true,
-		.gfp_mask = GFP_KERNEL,
-	};
-
-
-	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
-
-		zone = &pgdat->node_zones[zoneid];
-		if (!populated_zone(zone))
-			continue;
-
-		cc.zone = zone;
-
-		compact_zone(&cc, NULL);
-	}
+	compact_node(pgdat, true);
 }
 
-/* Compact all nodes in the system */
+/* Compact all zones of all nodes in the system */
 static void compact_nodes(void)
 {
 	int nid;
@@ -2958,7 +2938,7 @@ static void compact_nodes(void)
 	lru_add_drain_all();
 
 	for_each_online_node(nid)
-		compact_node(nid);
+		compact_node(NODE_DATA(nid), false);
 }
 
 static int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
@@ -3020,7 +3000,7 @@ static ssize_t compact_store(struct device *dev,
 		/* Flush pending updates to the LRU lists */
 		lru_add_drain_all();
 
-		compact_node(nid);
+		compact_node(NODE_DATA(nid), false);
 	}
 
 	return count;
-- 
2.27.0

Re: [PATCH] mm: compaction: refactor compact_node()

[Andrew Morton]

On Wed, 7 Feb 2024 17:58:41 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Refactor compact_node() to handle both proactive and synchronous compact
> memory, which cleanups code a bit.

Looks good.

> + * Compact all zones within a node till each zone's fragmentation score
> + * reaches within proactive compaction thresholds (as determined by the
> + * proactiveness tunable).
> + *
> + * It is possible that the function returns before reaching score targets
> + * due to various back-off conditions, such as, contention on per-node or
> + * per-zone locks.
> + */
> +static void proactive_compact_node(pg_data_t *pgdat)
>  {
> -	pg_data_t *pgdat = NODE_DATA(nid);
> -	int zoneid;
> -	struct zone *zone;
> -	struct compact_control cc = {
> -		.order = -1,
> -		.mode = MIGRATE_SYNC,
> -		.ignore_skip_hint = true,
> -		.whole_zone = true,
> -		.gfp_mask = GFP_KERNEL,
> -	};
> -
> -
> -	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
> -
> -		zone = &pgdat->node_zones[zoneid];
> -		if (!populated_zone(zone))
> -			continue;
> -
> -		cc.zone = zone;
> -
> -		compact_zone(&cc, NULL);
> -	}
> +	compact_node(pgdat, true);
>  }

I suggest that proactive_compact_node() be removed.  Move its comment
to compact_node(), add explanation of the bool argument to that comment
and open-code the call to compact_node() at proactive_compact_node()'s
sole call site.

[PATCH] mm: compaction: early termination in compact_nodes()

[Kefeng Wang]

No need to continue try compact memory if pending fatal signal,
allow loop termination earlier in compact_nodes().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/compaction.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index de882ecb61c5..52e75f8ac7e7 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2895,7 +2895,7 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
  * reaching score targets due to various back-off conditions, such as,
  * contention on per-node or per-zone locks.
  */
-static void compact_node(pg_data_t *pgdat, bool proactive)
+static int compact_node(pg_data_t *pgdat, bool proactive)
 {
 	int zoneid;
 	struct zone *zone;
@@ -2913,6 +2913,9 @@ static void compact_node(pg_data_t *pgdat, bool proactive)
 		if (!populated_zone(zone))
 			continue;
 
+		if (fatal_signal_pending(current))
+			return -EINTR;
+
 		cc.zone = zone;
 
 		compact_zone(&cc, NULL);
@@ -2924,18 +2927,25 @@ static void compact_node(pg_data_t *pgdat, bool proactive)
 					     cc.total_free_scanned);
 		}
 	}
+
+	return 0;
 }
 
 /* Compact all zones of all nodes in the system */
-static void compact_nodes(void)
+static int compact_nodes(void)
 {
-	int nid;
+	int ret, nid;
 
 	/* Flush pending updates to the LRU lists */
 	lru_add_drain_all();
 
-	for_each_online_node(nid)
-		compact_node(NODE_DATA(nid), false);
+	for_each_online_node(nid) {
+		ret = compact_node(NODE_DATA(nid), false);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
@@ -2981,9 +2991,9 @@ static int sysctl_compaction_handler(struct ctl_table *table, int write,
 		return -EINVAL;
 
 	if (write)
-		compact_nodes();
+		ret = compact_nodes();
 
-	return 0;
+	return ret;
 }
 
 #if defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
-- 
2.27.0

Re: [PATCH] mm: compaction: early termination in compact_nodes()

[Andrew Morton]

On Thu, 8 Feb 2024 10:25:08 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> No need to continue try compact memory if pending fatal signal,
> allow loop termination earlier in compact_nodes().

Seems sensible, but...  why?  Is there some problem which we can
demonstrate with the existing code?  In other words, does this change
provide any observable benefit under any circumstances?

Re: [PATCH] mm: compaction: early termination in compact_nodes()

[David Hildenbrand]

On 08.02.24 22:14, Andrew Morton wrote:
> On Thu, 8 Feb 2024 10:25:08 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> No need to continue try compact memory if pending fatal signal,
>> allow loop termination earlier in compact_nodes().
> 
> Seems sensible, but...  why?  Is there some problem which we can
> demonstrate with the existing code?  In other words, does this change
> provide any observable benefit under any circumstances?

I'd also be curious why the existing fatal_signal_pending() calls are 
insufficient.

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm: compaction: early termination in compact_nodes()

[Kefeng Wang]

On 2024/2/12 22:22, David Hildenbrand wrote:
> On 08.02.24 22:14, Andrew Morton wrote:
>> On Thu, 8 Feb 2024 10:25:08 +0800 Kefeng Wang 
>> <wangkefeng.wang@huawei.com> wrote:
>>
>>> No need to continue try compact memory if pending fatal signal,
>>> allow loop termination earlier in compact_nodes().
>>
>> Seems sensible, but...  why?  Is there some problem which we can
>> demonstrate with the existing code?  In other words, does this change
>> provide any observable benefit under any circumstances?
> 
> I'd also be curious why the existing fatal_signal_pending() calls are 
> insufficient.

The existing fatal_signal_pending() does make compact_zone() breakout of
the while loop, but it still enter the next zone/next nid, and some 
unnecessary functions(eg, lru_add_drain) called, no observable benefit
from test, it is just found from code inspection when refactor 
compact_node().

Re: [PATCH] mm: compaction: early termination in compact_nodes()

[Andrew Morton]

On Sat, 17 Feb 2024 10:55:10 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> On 2024/2/12 22:22, David Hildenbrand wrote:
> > On 08.02.24 22:14, Andrew Morton wrote:
> >> On Thu, 8 Feb 2024 10:25:08 +0800 Kefeng Wang 
> >> <wangkefeng.wang@huawei.com> wrote:
> >>
> >>> No need to continue try compact memory if pending fatal signal,
> >>> allow loop termination earlier in compact_nodes().
> >>
> >> Seems sensible, but...  why?  Is there some problem which we can
> >> demonstrate with the existing code?  In other words, does this change
> >> provide any observable benefit under any circumstances?
> > 
> > I'd also be curious why the existing fatal_signal_pending() calls are 
> > insufficient.
> 
> The existing fatal_signal_pending() does make compact_zone() breakout of
> the while loop, but it still enter the next zone/next nid, and some 
> unnecessary functions(eg, lru_add_drain) called, no observable benefit
> from test, it is just found from code inspection when refactor 

Fair enough.  I added the above words to the changelog (this material
should have been communicated in the original!) and I'll plan to move
this change into mm-stable next week unless someone stops me.

Re: [PATCH] mm: compaction: early termination in compact_nodes()

[Kefeng Wang]

On 2024/2/22 6:34, Andrew Morton wrote:
> On Sat, 17 Feb 2024 10:55:10 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> On 2024/2/12 22:22, David Hildenbrand wrote:
>>> On 08.02.24 22:14, Andrew Morton wrote:
>>>> On Thu, 8 Feb 2024 10:25:08 +0800 Kefeng Wang
>>>> <wangkefeng.wang@huawei.com> wrote:
>>>>
>>>>> No need to continue try compact memory if pending fatal signal,
>>>>> allow loop termination earlier in compact_nodes().
>>>>
>>>> Seems sensible, but...  why?  Is there some problem which we can
>>>> demonstrate with the existing code?  In other words, does this change
>>>> provide any observable benefit under any circumstances?
>>>
>>> I'd also be curious why the existing fatal_signal_pending() calls are
>>> insufficient.
>>
>> The existing fatal_signal_pending() does make compact_zone() breakout of
>> the while loop, but it still enter the next zone/next nid, and some
>> unnecessary functions(eg, lru_add_drain) called, no observable benefit
>> from test, it is just found from code inspection when refactor
> 
> Fair enough.  I added the above words to the changelog (this material
> should have been communicated in the original!) and I'll plan to move
> this change into mm-stable next week unless someone stops me.
Indeed, thanks for the update, you're so kind.