On Wed, 26 Feb 2025 at 09:12, Johannes Weiner <hannes@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202412290924.UTP7GH2Z-lkp@xxxxxxxxx/
> > > Signed-off-by: Zeng Jingxiang <linuszeng@xxxxxxxxxxx>
> > > ---
> > > 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
>
