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

[Shakeel Butt <shakeel.butt@linux.dev>]

On Fri, Feb 28, 2025 at 05:33:28PM +0100, Vlastimil Babka wrote:
> On 2/28/25 17:30, Vlastimil Babka wrote:
> > On 2/25/25 17:23, Shakeel Butt wrote:
> >> On Tue, Feb 25, 2025 at 11:30:20PM +0800, Jingxiang Zeng wrote:
> >>>  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.
> > 
> > BTW, if it's unlikely to be !NULL then it's not really efficient to call
> > kfree() unconditionally as it's a function call and those are more expensive
> > than inline check (especially with spectre mitigations).

Thanks, that warning gave me the impression that unconditional kfree() is
always better.

> 
> OTOH making kfree() do the check inline would bloat all callers, and most of
> them are likely !NULL.
> We could perhaps have a kfree_unlikely() variant doing an inline
> "if unlikely(obj) kfree()" if enough places do it.
> 

This makes sense.