mm/mglru: fix memcg protection for global proactive reclaim

From: Ridong Chen <[email protected]>

memory.min/low is silently bypassed for MGLRU during global proactive reclaim (writing to the root memory.reclaim). It can be reproduced as follows:

  # echo 7 > /sys/kernel/mm/lru_gen/enabled
  # cd /sys/fs/cgroup
  # mkdir -p a/b
  # echo 100M > a/memory.min
  # echo +memory > a/cgroup.subtree_control
  # echo 100M > a/b/memory.min
  # echo $$ > a/b/cgroup.procs
  # dd if=/dev/zero of=/tmp/testfile bs=1M count=200
  # cat a/b/memory.current
  222650368
  # echo 500M > memory.reclaim
  -bash: echo: write error: Resource temporarily unavailable
  # cat a/b/memory.current
  6070272

memory.min is 100M, yet reclaim drops a/b down to 6M, breaking the protection. The traditional LRU is not affected.

Commit 30d77b7eef01 ("mm/mglru: fix ineffective protection calculation") computes the memcg protection in lru_gen_age_node(), which only runs for kswapd. Global proactive reclaim reaches shrink_one() through lru_gen_shrink_node() without any aging step, so emin/elow is not couputed,

Factor the tree traversal out into update_memcg_protection() and call it from lru_gen_shrink_node() for the non-kswapd path, so the protection is computed before shrinking. kswapd keeps computing it in lru_gen_age_node(), which also needs it for the min_ttl OOM check.

---
 mm/vmscan.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

index 06e103f9781e..dc3549583e97 100644
@@ -4122,6 +4122,16 @@ static bool lruvec_is_reclaimable(struct lruvec *lruvec, struct scan_control *sc
 	return time_is_before_jiffies(birth + min_ttl);
 }
 
+static void update_memcg_protection(void)
+{
+	struct mem_cgroup *memcg;
+
+	memcg = mem_cgroup_iter(NULL, NULL, NULL);
+	do {
+		mem_cgroup_calculate_protection(NULL, memcg);
+	} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)));
+}
+
 /* to protect the working set of the last N jiffies */
 static unsigned long lru_gen_min_ttl __read_mostly;
 

@@ -4135,15 +4145,19 @@ static void lru_gen_age_node(struct pglist_data *pgdat, struct scan_control *sc)
 
 	set_initial_priority(pgdat, sc);
 
-	memcg = mem_cgroup_iter(NULL, NULL, NULL);
-	do {
-		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
+	update_memcg_protection();
 
-		mem_cgroup_calculate_protection(NULL, memcg);
+	if (min_ttl) {
+		memcg = mem_cgroup_iter(NULL, NULL, NULL);
+		do {
+			struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
 
-		if (!reclaimable)
-			reclaimable = lruvec_is_reclaimable(lruvec, sc, min_ttl);
-	} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)));
+			if (lruvec_is_reclaimable(lruvec, sc, min_ttl)) {
+				reclaimable = true;
+				break;
+			}
+		} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)));
+	}
 
 	/*
 	 * The main goal is to OOM kill if every generation from all memcgs is

@@ -5004,7 +5018,7 @@ static int shrink_one(struct lruvec *lruvec, struct scan_control *sc)
 	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
 
-	/* lru_gen_age_node() called mem_cgroup_calculate_protection() */
+	/* update_memcg_protection() computed the protection */
 	if (mem_cgroup_below_min(NULL, memcg))
 		return MEMCG_LRU_YOUNG;
 

@@ -5151,8 +5165,11 @@ static void lru_gen_shrink_node(struct pglist_data *pgdat, struct scan_control *
 
 	set_initial_priority(pgdat, sc);
 
+	/* kswapd called update_memcg_protection in lru_gen_age_node */
 	if (current_is_kswapd())
 		sc->nr_reclaimed = 0;
+	else
+		update_memcg_protection();
 
 	if (mem_cgroup_disabled())
 		shrink_one(&pgdat->__lruvec, sc);

2.34.1

Do we want cc:stable on this fix?

Sashiko said a couple of things - the memcg ref leak looks real:

	https://sashiko.dev/#/patchset/[email protected]

Sashiko said:

When breaking out of the loop early here, do we need to call mem_cgroup_iter_break(NULL, memcg) to release the reference? Since mem_cgroup_iter() holds a reference to the active cgroup css, exiting without dropping it could cause memory cgroups to leak and accumulate over time, eventually leading to kernel memory exhaustion.
[ ... ]

This is a bug introduced by this patch, and we will fix it.

Regarding the performance regression:

Placing update_memcg_protection() inside the lru_gen_shrink_node() non-kswapd path forces every direct reclaimer into an unbounded full cgroup tree walk.
Will this cause severe performance regressions during global memory pressure? All allocating tasks entering global direct reclaim would concurrently traverse the entire memcg tree. This could lead to massive css->refcnt cacheline bouncing and system latency spikes, scaling negatively with the number of memory cgroups.
Could this full tree walk be optimized or deferred so direct reclaimers avoid iterating every single cgroup?

Since MGLRU global reclaim does not iterate over memcgs in the same way traditional LRU does (which traverses the hierarchy from top to bottom), it appears we are currently required to walk the full tree, similar to what kswapd reclaim does.

Does anyone have a better approach in mind?

Best regards
Ridong