[v2,3/6] mm/list_lru: code clean up for reparenting

Commit Message

Kairui Song Sept. 25, 2024, 5:10 p.m. UTC

From: Kairui Song <[e-mail]>

No feature change, just change of code structure and fix comment.

The list lrus are not empty until memcg_reparent_list_lru_node() calls
are all done, so the comments in memcg_offline_kmem were slightly
inaccurate.

Signed-off-by: Kairui Song <[e-mail]>
Reviewed-by: Muchun Song <[e-mail]>
---
 mm/list_lru.c   | 39 +++++++++++++++++----------------------
 mm/memcontrol.c |  7 -------
 2 files changed, 17 insertions(+), 29 deletions(-)

Comments

Shakeel Butt Sept. 26, 2024, 2:34 p.m. UTC | #1

On Thu, Sep 26, 2024 at 01:10:17AM GMT, Kairui Song wrote:
> From: Kairui Song <[e-mail]>
> 
> No feature change, just change of code structure and fix comment.
> 
> The list lrus are not empty until memcg_reparent_list_lru_node() calls
> are all done, so the comments in memcg_offline_kmem were slightly
> inaccurate.
> 
> Signed-off-by: Kairui Song <[e-mail]>
> Reviewed-by: Muchun Song <[e-mail]>

Acked-by: Shakeel Butt <[e-mail]>

Patch

diff --git a/mm/list_lru.c b/mm/list_lru.c
index a798e7624f69..b54f092d4d65 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -421,35 +421,16 @@  static void memcg_reparent_list_lru_node(struct list_lru *lru, int nid,
 	spin_unlock_irq(&nlru->lock);
 }
 
-static void memcg_reparent_list_lru(struct list_lru *lru,
-				    int src_idx, struct mem_cgroup *dst_memcg)
-{
-	int i;
-
-	for_each_node(i)
-		memcg_reparent_list_lru_node(lru, i, src_idx, dst_memcg);
-
-	memcg_list_lru_free(lru, src_idx);
-}
-
 void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *parent)
 {
 	struct cgroup_subsys_state *css;
 	struct list_lru *lru;
-	int src_idx = memcg->kmemcg_id;
+	int src_idx = memcg->kmemcg_id, i;
 
 	/*
 	 * Change kmemcg_id of this cgroup and all its descendants to the
 	 * parent's id, and then move all entries from this cgroup's list_lrus
 	 * to ones of the parent.
-	 *
-	 * After we have finished, all list_lrus corresponding to this cgroup
-	 * are guaranteed to remain empty. So we can safely free this cgroup's
-	 * list lrus in memcg_list_lru_free().
-	 *
-	 * Changing ->kmemcg_id to the parent can prevent memcg_list_lru_alloc()
-	 * from allocating list lrus for this cgroup after memcg_list_lru_free()
-	 * call.
 	 */
 	rcu_read_lock();
 	css_for_each_descendant_pre(css, &memcg->css) {
@@ -460,9 +441,23 @@  void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
 	}
 	rcu_read_unlock();
 
+	/*
+	 * With kmemcg_id set to parent, holding the lock of each list_lru_node
+	 * below can prevent list_lru_{add,del,isolate} from touching the lru,
+	 * safe to reparent.
+	 */
 	mutex_lock(&list_lrus_mutex);
-	list_for_each_entry(lru, &memcg_list_lrus, list)
-		memcg_reparent_list_lru(lru, src_idx, parent);
+	list_for_each_entry(lru, &memcg_list_lrus, list) {
+		for_each_node(i)
+			memcg_reparent_list_lru_node(lru, i, src_idx, parent);
+
+		/*
+		 * Here all list_lrus corresponding to the cgroup are guaranteed
+		 * to remain empty, we can safely free this lru, any further
+		 * memcg_list_lru_alloc() call will simply bail out.
+		 */
+		memcg_list_lru_free(lru, src_idx);
+	}
 	mutex_unlock(&list_lrus_mutex);
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7845c64a2c57..8e90aa026c47 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3099,13 +3099,6 @@  static void memcg_offline_kmem(struct mem_cgroup *memcg)
 		parent = root_mem_cgroup;
 
 	memcg_reparent_objcgs(memcg, parent);
-
-	/*
-	 * After we have finished memcg_reparent_objcgs(), all list_lrus
-	 * corresponding to this cgroup are guaranteed to remain empty.
-	 * The ordering is imposed by list_lru_node->lock taken by
-	 * memcg_reparent_list_lrus().
-	 */
 	memcg_reparent_list_lrus(memcg, parent);
 }