On 2024/6/25 01:53, Kairui Song wrote:
From: Kairui Song <kasong@xxxxxxxxxxx>
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 <kasong@xxxxxxxxxxx>
---
mm/list_lru.c | 39 +++++++++++++++++----------------------
mm/memcontrol.c | 7 -------
2 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 9d9ec8661354..4c619857e916 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -405,35 +405,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) {
@@ -444,9 +425,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 lru lock below can
The word of "below" confuses me a bit. "lru lock below" refers to 1)
"list_lrus_mutex"
or the 2) lock in "struct list_lru_node"?
I think it is 2), right?
The chnges LGTM.
Reviewed-by: Muchun Song <muchun.song@xxxxxxxxx>
Thanks.
+ * 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 71fe2a95b8bd..fc35c1d3f109 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4187,13 +4187,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);
}
#else
