Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely

[jingxiang zeng <jingxiangzeng.cas@gmail.com>]

On Wed, 26 Feb 2025 at 09:12, Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Tue, Feb 25, 2025 at 08:23:12AM -0800, Shakeel Butt wrote:
> > On Tue, Feb 25, 2025 at 11:30:20PM +0800, Jingxiang Zeng wrote:
> > > From: Zeng Jingxiang <linuszeng@tencent.com>
> > >
> > > In the following memcg_list_lru_alloc() function, mlru here is almost
> > > always NULL, so in most cases this should save a function call, mark
> > > mlru as unlikely to optimize the code.
> > >         do {
> > >                 xas_lock_irqsave(&xas, flags);
> > >                 if (!xas_load(&xas) && !css_is_dying(&pos->css)) {
> > >                         xas_store(&xas, mlru);
> > >                         if (!xas_error(&xas))
> > >                                 mlru = NULL;
> > >                 }
> > >                 xas_unlock_irqrestore(&xas, flags);
> > >         } while (xas_nomem(&xas, GFP_KERNEL));
> > > >       if (mlru)
> > >                 kfree(mlru);
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202412290924.UTP7GH2Z-lkp@intel.com/
> > > Signed-off-by: Zeng Jingxiang <linuszeng@tencent.com>
> > > ---
> > >  mm/list_lru.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/list_lru.c b/mm/list_lru.c
> > > index 064d2018e265..e7e13513ff8e 100644
> > > --- a/mm/list_lru.c
> > > +++ b/mm/list_lru.c
> > > @@ -552,7 +552,7 @@ static int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru)
> > >                     }
> > >                     xas_unlock_irqrestore(&xas, flags);
> > >             } while (xas_nomem(&xas, GFP_KERNEL));
> > > -           if (mlru)
> > > +           if (unlikely(mlru))
> > >                     kfree(mlru);
> >
> > The report is saying not to check at all. So, just remove the check and
> > simply call kfree(mlru) as it handles the NULL check efficiently.
>
> I actually like it in this case. It's an "active comment" that this
> only happens in the failure case and we don't routinely free here.
>
> That said, does it have to free the mlru inside the loop at all? If
> the tree insertion fails, why not reuse it for the next attempt?

I agree with this implementation; reusing the mlru for the next attempt
when the tree insertion fails is more appropriate.
>
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index 7d69434c70e0..490473af3122 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -510,7 +510,7 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
>                          gfp_t gfp)
>  {
>         unsigned long flags;
> -       struct list_lru_memcg *mlru;
> +       struct list_lru_memcg *mlru = NULL;
>         struct mem_cgroup *pos, *parent;
>         XA_STATE(xas, &lru->xa, 0);
>
> @@ -535,9 +535,11 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
>                         parent = parent_mem_cgroup(pos);
>                 }
>
> -               mlru = memcg_init_list_lru_one(lru, gfp);
> -               if (!mlru)
> -                       return -ENOMEM;
> +               if (!mlru) {
> +                       mlru = memcg_init_list_lru_one(lru, gfp);
> +                       if (!mlru)
> +                               return -ENOMEM;
> +               }
>                 xas_set(&xas, pos->kmemcg_id);
>                 do {
>                         xas_lock_irqsave(&xas, flags);
> @@ -548,10 +550,11 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
>                         }
>                         xas_unlock_irqrestore(&xas, flags);
>                 } while (xas_nomem(&xas, gfp));
> -               if (mlru)
> -                       kfree(mlru);
>         } while (pos != memcg && !css_is_dying(&pos->css));
>
> +       if (unlikely(mlru))
> +               kfree(mlru);
> +
>         return xas_error(&xas);
>  }
>  #else
>