Re: [PATCH 4/7] mm/list_lru: code clean up for reparenting

[Kairui Song <ryncsn@gmail.com>]

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.