| Message ID | 20240624175313.47329-5-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Split list_lru lock into per-cgroup scope | expand |
On 2024/6/25 01:53, Kairui Song wrote: > From: Kairui Song <kasong@tencent.com> > > 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@tencent.com> > --- > 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@linux.dev> 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
On Mon, Jul 15, 2024 at 5:12 PM Muchun Song <muchun.song@linux.dev> wrote: > On 2024/6/25 01:53, Kairui Song wrote: > > From: Kairui Song <kasong@tencent.com> > > > > 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@tencent.com> > > --- > > 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? Yes, that's 2). > The chnges LGTM. > > Reviewed-by: Muchun Song <muchun.song@linux.dev> Thanks, I can make the comments more concrete in the next version.
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 + * 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