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

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

[Jingxiang Zeng]

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);
 		set_active_memcg(cur);
 	} while (pos != memcg && !css_is_dying(&pos->css));
-- 
2.43.5

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

[Shakeel Butt]

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.

>  		set_active_memcg(cur);
>  	} while (pos != memcg && !css_is_dying(&pos->css));
> -- 
> 2.43.5
> 
> 

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

[Johannes Weiner]

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

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

[jingxiang zeng]

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
>

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

[Shakeel Butt]

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?

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

[jingxiang zeng]

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.

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

[Vlastimil Babka]

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).

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

[Vlastimil Babka]

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).

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.

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

[Shakeel Butt]

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.