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

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

On Thu, 27 Feb 2025 at 05:08, Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> On Tue, Feb 25, 2025 at 08:11:47PM -0500, Johannes Weiner 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?
> >
> > 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);
>
> Yup this looks good. Will unlikely() shutup the warning from bot?
>
I verified it locally using the COCCI test，COCCI check no longer
reports the NULL check error.