* [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
@ 2025-02-25 15:30 Jingxiang Zeng
2025-02-25 16:23 ` Shakeel Butt
0 siblings, 1 reply; 9+ messages in thread
From: Jingxiang Zeng @ 2025-02-25 15:30 UTC (permalink / raw)
To: linux-mm
Cc: akpm, hannes, mhocko, roman.gushchin, shakeel.butt, muchun.song,
chengming.zhou, kasong, Zeng Jingxiang, kernel test robot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-25 15:30 [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely Jingxiang Zeng
@ 2025-02-25 16:23 ` Shakeel Butt
2025-02-26 1:11 ` Johannes Weiner
2025-02-28 16:30 ` Vlastimil Babka
0 siblings, 2 replies; 9+ messages in thread
From: Shakeel Butt @ 2025-02-25 16:23 UTC (permalink / raw)
To: Jingxiang Zeng
Cc: linux-mm, akpm, hannes, mhocko, roman.gushchin, muchun.song,
chengming.zhou, kasong, kernel test robot
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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-25 16:23 ` Shakeel Butt
@ 2025-02-26 1:11 ` Johannes Weiner
2025-02-26 2:09 ` jingxiang zeng
2025-02-26 21:08 ` Shakeel Butt
2025-02-28 16:30 ` Vlastimil Babka
1 sibling, 2 replies; 9+ messages in thread
From: Johannes Weiner @ 2025-02-26 1:11 UTC (permalink / raw)
To: Shakeel Butt
Cc: Jingxiang Zeng, linux-mm, akpm, mhocko, roman.gushchin,
muchun.song, chengming.zhou, kasong, kernel test robot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-26 1:11 ` Johannes Weiner
@ 2025-02-26 2:09 ` jingxiang zeng
2025-02-26 21:08 ` Shakeel Butt
1 sibling, 0 replies; 9+ messages in thread
From: jingxiang zeng @ 2025-02-26 2:09 UTC (permalink / raw)
To: Johannes Weiner
Cc: Shakeel Butt, Jingxiang Zeng, linux-mm, akpm, mhocko,
roman.gushchin, muchun.song, chengming.zhou, kasong,
kernel test robot
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-26 1:11 ` Johannes Weiner
2025-02-26 2:09 ` jingxiang zeng
@ 2025-02-26 21:08 ` Shakeel Butt
2025-02-27 8:03 ` jingxiang zeng
1 sibling, 1 reply; 9+ messages in thread
From: Shakeel Butt @ 2025-02-26 21:08 UTC (permalink / raw)
To: Johannes Weiner
Cc: Jingxiang Zeng, linux-mm, akpm, mhocko, roman.gushchin,
muchun.song, chengming.zhou, kasong, kernel test robot
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-26 21:08 ` Shakeel Butt
@ 2025-02-27 8:03 ` jingxiang zeng
0 siblings, 0 replies; 9+ messages in thread
From: jingxiang zeng @ 2025-02-27 8:03 UTC (permalink / raw)
To: Shakeel Butt
Cc: Johannes Weiner, Jingxiang Zeng, linux-mm, akpm, mhocko,
roman.gushchin, muchun.song, chengming.zhou, kasong,
kernel test robot
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-25 16:23 ` Shakeel Butt
2025-02-26 1:11 ` Johannes Weiner
@ 2025-02-28 16:30 ` Vlastimil Babka
2025-02-28 16:33 ` Vlastimil Babka
1 sibling, 1 reply; 9+ messages in thread
From: Vlastimil Babka @ 2025-02-28 16:30 UTC (permalink / raw)
To: Shakeel Butt, Jingxiang Zeng
Cc: linux-mm, akpm, hannes, mhocko, roman.gushchin, muchun.song,
chengming.zhou, kasong, kernel test robot
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).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-28 16:30 ` Vlastimil Babka
@ 2025-02-28 16:33 ` Vlastimil Babka
2025-02-28 18:48 ` Shakeel Butt
0 siblings, 1 reply; 9+ messages in thread
From: Vlastimil Babka @ 2025-02-28 16:33 UTC (permalink / raw)
To: Shakeel Butt, Jingxiang Zeng
Cc: linux-mm, akpm, hannes, mhocko, roman.gushchin, muchun.song,
chengming.zhou, kasong, kernel test robot
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely
2025-02-28 16:33 ` Vlastimil Babka
@ 2025-02-28 18:48 ` Shakeel Butt
0 siblings, 0 replies; 9+ messages in thread
From: Shakeel Butt @ 2025-02-28 18:48 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Jingxiang Zeng, linux-mm, akpm, hannes, mhocko, roman.gushchin,
muchun.song, chengming.zhou, kasong, kernel test robot
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-02-28 18:48 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-25 15:30 [PATCH] mm/list_lru: make the case where mlru is NULL as unlikely Jingxiang Zeng
2025-02-25 16:23 ` Shakeel Butt
2025-02-26 1:11 ` Johannes Weiner
2025-02-26 2:09 ` jingxiang zeng
2025-02-26 21:08 ` Shakeel Butt
2025-02-27 8:03 ` jingxiang zeng
2025-02-28 16:30 ` Vlastimil Babka
2025-02-28 16:33 ` Vlastimil Babka
2025-02-28 18:48 ` Shakeel Butt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox