Re: [PATCH v5 update 29/32] mm: memcontrol: prepare for reparenting non-hierarchical stats

[Yosry Ahmed <yosry@kernel.org>]

On Tue, Mar 03, 2026 at 11:08:56AM +0800, Qi Zheng wrote:
> Hi Yosry,
[..] 
> > 
> > I don't think we should end up with two copies of
> > __mod_memcg_state/mod_memcg_state() and
> > __mod_memcg_lruvec_state/mod_memcg_lruvec_state(). I meant to refactor
> > mod_memcg_state() to call __mod_memcg_state(), where the latter does
> > not call get_non_dying_memcg_{start/end}(). Same for
> > mod_memcg_lruvec_state().
> 
> Okay, like the following? But this would require modifications to
> [PATCH v5 31/32]. If there are no problems, I will send the updated
> patch to [PATCH v5 29/32] and [PATCH v5 31/32].

I cannot apply the diff, seems a bit corrupted.

But ideally, instead of a @reparent argument, we just have
__mod_memcg_lruvec_state() and __mod_memcg_state() do the work without
getting parent of dead memcgs, and then mod_memcg_lruvec_state() and
mod_memcg_state() just call them after get_non_dying_memcg_start().

What about this (untested), it should apply on top of 'mm: memcontrol:
eliminate the problem of dying memory cgroup for LRU folios' in mm-new,
so maybe it needs to be broken down across different patches:

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 753d76e96cc67..f0d55e1f9c49a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -527,7 +527,7 @@ unsigned long lruvec_page_state_local(struct lruvec *lruvec,
 }
 
 #ifdef CONFIG_MEMCG_V1
-static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
+static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
 				     enum node_stat_item idx, int val);
 
 void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
@@ -536,16 +536,17 @@ void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
 	int i = memcg_stats_index(idx);
 	int nid;
 
-	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
-		return;
-
 	for_each_node(nid) {
 		struct lruvec *child_lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(nid));
 		struct lruvec *parent_lruvec = mem_cgroup_lruvec(parent, NODE_DATA(nid));
 		unsigned long value = lruvec_page_state_local(child_lruvec, idx);
+		struct mem_cgroup_per_node *child_pn, *parent_pn;
 
-		__mod_memcg_lruvec_state(child_lruvec, idx, -value);
-		__mod_memcg_lruvec_state(parent_lruvec, idx, value);
+		child_pn = container_of(child_lruvec, struct mem_cgroup_per_node, lruvec);
+		parent_pn = container_of(parent_lruvec, struct mem_cgroup_per_node, lruvec);
+
+		__mod_memcg_lruvec_state(child_pn, idx, -value);
+		__mod_memcg_lruvec_state(parent_pn, idx, value);
 	}
 }
 #endif
@@ -831,39 +832,42 @@ static inline void get_non_dying_memcg_end(void)
 }
 #endif
 
-/**
- * mod_memcg_state - update cgroup memory statistics
- * @memcg: the memory cgroup
- * @idx: the stat item - can be enum memcg_stat_item or enum node_stat_item
- * @val: delta to add to the counter, can be negative
- */
-void mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
-		       int val)
+static void __mod_memcg_state(struct mem_cgroup *memcg,
+			      enum memcg_stat_item idx, int val)
 {
 	int i = memcg_stats_index(idx);
 	int cpu;
 
-	if (mem_cgroup_disabled())
-		return;
-
 	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
 		return;
 
 	cpu = get_cpu();
 
-	memcg = get_non_dying_memcg_start(memcg);
-
 	this_cpu_add(memcg->vmstats_percpu->state[i], val);
 	val = memcg_state_val_in_pages(idx, val);
 	memcg_rstat_updated(memcg, val, cpu);
-
-	get_non_dying_memcg_end();
-
 	trace_mod_memcg_state(memcg, idx, val);
 
 	put_cpu();
 }
 
+/**
+ * mod_memcg_state - update cgroup memory statistics
+ * @memcg: the memory cgroup
+ * @idx: the stat item - can be enum memcg_stat_item or enum node_stat_item
+ * @val: delta to add to the counter, can be negative
+ */
+void mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
+		     int val)
+{
+	if (mem_cgroup_disabled())
+		return;
+
+	memcg = get_non_dying_memcg_start(memcg);
+	__mod_memcg_state(memcg, idx, val);
+	get_non_dying_memcg_end();
+}
+
 #ifdef CONFIG_MEMCG_V1
 /* idx can be of type enum memcg_stat_item or node_stat_item. */
 unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
@@ -882,35 +886,26 @@ unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
 	return x;
 }
 
-static void __mod_memcg_state(struct mem_cgroup *memcg,
-			      enum memcg_stat_item idx, int val)
+void reparent_memcg_state_local(struct mem_cgroup *memcg,
+				struct mem_cgroup *parent, int idx)
 {
 	int i = memcg_stats_index(idx);
-	int cpu;
-
-	if (mem_cgroup_disabled())
-		return;
-
-	cpu = get_cpu();
-
-	this_cpu_add(memcg->vmstats_percpu->state[i], val);
-	val = memcg_state_val_in_pages(idx, val);
-	memcg_rstat_updated(memcg, val, cpu);
-	trace_mod_memcg_state(memcg, idx, val);
+	unsigned long value = memcg_page_state_local(memcg, idx);
 
-	put_cpu();
+	__mod_memcg_state(memcg, idx, -value);
+	__mod_memcg_state(parent, idx, value);
 }
+#endif
 
-static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
+static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
 				     enum node_stat_item idx, int val)
 {
-	struct mem_cgroup_per_node *pn;
-	struct mem_cgroup *memcg;
+	struct mem_cgroup *memcg = pn->memcg;
 	int i = memcg_stats_index(idx);
 	int cpu;
 
-	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
-	memcg = pn->memcg;
+	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
+		return;
 
 	cpu = get_cpu();
 
@@ -927,20 +922,6 @@ static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
 	put_cpu();
 }
 
-void reparent_memcg_state_local(struct mem_cgroup *memcg,
-				struct mem_cgroup *parent, int idx)
-{
-	int i = memcg_stats_index(idx);
-	unsigned long value = memcg_page_state_local(memcg, idx);
-
-	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
-		return;
-
-	__mod_memcg_state(memcg, idx, -value);
-	__mod_memcg_state(parent, idx, value);
-}
-#endif
-
 static void mod_memcg_lruvec_state(struct lruvec *lruvec,
 				     enum node_stat_item idx,
 				     int val)
@@ -948,32 +929,13 @@ static void mod_memcg_lruvec_state(struct lruvec *lruvec,
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
 	struct mem_cgroup_per_node *pn;
 	struct mem_cgroup *memcg;
-	int i = memcg_stats_index(idx);
-	int cpu;
-
-	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
-		return;
 
 	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
-	memcg = pn->memcg;
-
-	cpu = get_cpu();
-
-	memcg = get_non_dying_memcg_start(memcg);
+	memcg = get_non_dying_memcg_start(pn->memcg);
 	pn = memcg->nodeinfo[pgdat->node_id];
-
-	/* Update memcg */
-	this_cpu_add(memcg->vmstats_percpu->state[i], val);
-	/* Update lruvec */
-	this_cpu_add(pn->lruvec_stats_percpu->state[i], val);
-	val = memcg_state_val_in_pages(idx, val);
-	memcg_rstat_updated(memcg, val, cpu);
-
+	__mod_memcg_lruvec_state(pn, idx, val);
 	get_non_dying_memcg_end();
 
-	trace_mod_memcg_lruvec_state(memcg, idx, val);
-
-	put_cpu();
 }
 
 /