Date:   Tue, 18 Apr 2023 09:58:54 -0700
From:   Douglas Anderson <dianders@...omium.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Vlastimil Babka <vbabka@...e.cz>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Yu Zhao <yuzhao@...gle.com>, Ying <ying.huang@...el.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Douglas Anderson <dianders@...omium.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: [RFC PATCH] mm, compaction: kcompactd work shouldn't count towards memory PSI

When the main kcompactd thread is doing compaction then it's always
proactive compaction. This is a little confusing because kcompactd has
two phases and one of them is called the "proactive" phase.
Specifically:
* Phase 1 (the "non-proactive" phase): we've been told by someone else
  that it would be a good idea to try to compact memory.
* Phase 2 (the "proactive" phase): we analyze memory fragmentation
  ourselves and compact if it looks fragmented.

>From the context of kcompactd, the above naming makes sense. However,
from the context of the kernel as a whole both phases are "proactive"
because in both cases we're trying compact memory ahead of time and
we're not actually blocking (stalling) any task who is trying to use
memory.

Specifically, if any task is actually blocked needing memory to be
compacted then it will be in direct reclaim. That won't block waiting
on kcompactd task but instead call try_to_compact_pages() directly.
The caller of that direct compaction, __alloc_pages_direct_compact(),
already marks itself as counting towards PSI.

Sanity checking by looking at this from another perspective, we can
look at all the people who explicitly ask kcompactd to do a reclaim by
calling wakeup_kcompactd(). That leads us to 3 places in vmscan.c.
Those are all requests from kswapd, which is also a "proactive"
mechanism in the kernel (tasks aren't blocked waiting for it).

Fixes: eb414681d5a0 ("psi: pressure stall information for CPU, memory, and IO")
Signed-off-by: Douglas Anderson <dianders@...omium.org>
---
I stumbled upon this while researching for a different patch [1].
Although both the other patch and this one affect kcompactd, they are
otherwise unrelated. It can be noted that ${SUBJECT} patch was created
solely by code inspection. I don't have any specific test cases that
are made better by it, the code just didn't seem quite right to me.

My knowledge of the memory subsystem is shaky at best, so please take
this patch with a grain of salt. If you're a memory system expert and
this patch looks totally misguided to you then it probably is. ;-)

[1] https://lore.kernel.org/r/20230413182313.RFC.1.Ia86ccac02a303154a0b8bc60567e7a95d34c96d3@changeid

 mm/compaction.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 5a9501e0ae01..5a8d78b506e4 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -22,7 +22,6 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 #include <linux/page_owner.h>
-#include <linux/psi.h>
 #include "internal.h"
 
 #ifdef CONFIG_COMPACTION
@@ -2954,8 +2953,6 @@ static int kcompactd(void *p)
 	pgdat->kcompactd_highest_zoneidx = pgdat->nr_zones - 1;
 
 	while (!kthread_should_stop()) {
-		unsigned long pflags;
-
 		/*
 		 * Avoid the unnecessary wakeup for proactive compaction
 		 * when it is disabled.
@@ -2967,9 +2964,8 @@ static int kcompactd(void *p)
 			kcompactd_work_requested(pgdat), timeout) &&
 			!pgdat->proactive_compact_trigger) {
 
-			psi_memstall_enter(&pflags);
 			kcompactd_do_work(pgdat);
-			psi_memstall_leave(&pflags);
+
 			/*
 			 * Reset the timeout value. The defer timeout from
 			 * proactive compaction is lost here but that is fine
-- 
2.40.0.634.g4ca3ef3211-goog

Powered by blists - more mailing lists