On Mon, Jul 15, 2024 at 5:12 PM Muchun Song <muchun.song@xxxxxxxxx> wrote:
> 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?
Yes, that's 2).
> The chnges LGTM.
>
> Reviewed-by: Muchun Song <muchun.song@xxxxxxxxx>
Thanks, I can make the comments more concrete in the next version.
