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

[Johannes Weiner <hannes@cmpxchg.org>]

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);
+
 	return xas_error(&xas);
 }
 #else