Re: [PATCH] mm/page_idle: call folio_test_lru() after folio_get()

Re: [PATCH] mm/page_idle: call folio_test_lru() after folio_get()

[Lorenzo Stoakes (ARM)]

On Wed, Jul 22, 2026 at 05:26:42PM +0800, Jiale Yao wrote:
> page_idle_get_folio() speculatively calls folio_test_lru() before
> folio_try_get(). The folio can get freed and reallocated to a tail page
> in the meantime. In that case, VM_BUG_ON_PGFLAGS() in
> const_folio_flags() can be triggered. Remove the speculative call.
>
> Also mark the folio_test_lru() check right after folio_try_get() success
> as no more unlikely.

Slightly strange wording but not sure why you're doing that? It is
generally unlikely a given folio will be !LRU right?

>
> This is a sibling-path bug: damon_get_folio() was copied from this
> function with the same flawed pattern. Commit d6b8b02a27b3
> ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed
> damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN
> (strict mode) confirms the data race on the folio flags:
>
>   BUG: KCSAN: data-race in ... / percpu_counter_add_batch
>     page_idle_get_folio+0x7a/0x2d0
>     page_idle_bitmap_read+0xc9/0x220
>
> Signed-off-by: Jiale Yao <yaojiale02@163.com>

Yeah generally this seems obviously correct (TM), if we can't be sure a
folio is kept around any other way to the extent we're doing
folio_try_get() we should gate any actual interactions with the folio on
succeeding the get first...!

With the unlikely thing changed, LGTM so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> ---
>  mm/page_idle.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/page_idle.c b/mm/page_idle.c
> index 9c67cbac2965..29ee18f0e8ce 100644
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -40,9 +40,9 @@ static struct folio *page_idle_get_folio(unsigned long pfn)
>  		return NULL;
>
>  	folio = page_folio(page);
> -	if (!folio_test_lru(folio) || !folio_try_get(folio))

I guess this was meant as a racey check...

> +	if (!folio_try_get(folio))
>  		return NULL;
> -	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
> +	if (unlikely(page_folio(page) != folio) || !folio_test_lru(folio)) {

As above, not sure why you're changing this? Seems unrelated, I'd just keep
it as it was.

>  		folio_put(folio);
>  		folio = NULL;
>  	}
> --
> 2.34.1
>

Cheers, Lorenzo

Re:Re: [PATCH] mm/page_idle: call folio_test_lru() after folio_get()

[jiale yao]

page_idle_get_folio() speculatively calls folio_test_lru() before
folio_try_get(). The folio can get freed and reallocated to a tail page
in the meantime. In that case, VM_BUG_ON_PGFLAGS() in
const_folio_flags() can be triggered. Remove the speculative call.

This is a sibling-path bug: damon_get_folio() was copied from this
function with the same flawed pattern. Commit d6b8b02a27b3
("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed
damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN
(strict mode) confirms the data race on the folio flags:

  BUG: KCSAN: data-race in ... / percpu_counter_add_batch
    page_idle_get_folio+0x7a/0x2d0
    page_idle_bitmap_read+0xc9/0x220

Signed-off-by: Jiale Yao <yaojiale02@163.com>
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
---
 mm/page_idle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_idle.c b/mm/page_idle.c
index 9c67cbac2965..b6c26a0ba3d5 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -40,7 +40,7 @@ static struct folio *page_idle_get_folio(unsigned long pfn)
 		return NULL;
 
 	folio = page_folio(page);
-	if (!folio_test_lru(folio) || !folio_try_get(folio))
+	if (!folio_try_get(folio))
 		return NULL;
 	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
 		folio_put(folio);
-- 
2.34.1





At 2026-07-22 17:56:13, "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:
>On Wed, Jul 22, 2026 at 05:26:42PM +0800, Jiale Yao wrote:
>> page_idle_get_folio() speculatively calls folio_test_lru() before
>> folio_try_get(). The folio can get freed and reallocated to a tail page
>> in the meantime. In that case, VM_BUG_ON_PGFLAGS() in
>> const_folio_flags() can be triggered. Remove the speculative call.
>>
>> Also mark the folio_test_lru() check right after folio_try_get() success
>> as no more unlikely.
>
>Slightly strange wording but not sure why you're doing that? It is
>generally unlikely a given folio will be !LRU right?
>
>>
>> This is a sibling-path bug: damon_get_folio() was copied from this
>> function with the same flawed pattern. Commit d6b8b02a27b3
>> ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed
>> damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN
>> (strict mode) confirms the data race on the folio flags:
>>
>>   BUG: KCSAN: data-race in ... / percpu_counter_add_batch
>>     page_idle_get_folio+0x7a/0x2d0
>>     page_idle_bitmap_read+0xc9/0x220
>>
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>
>Yeah generally this seems obviously correct (TM), if we can't be sure a
>folio is kept around any other way to the extent we're doing
>folio_try_get() we should gate any actual interactions with the folio on
>succeeding the get first...!
>
>With the unlikely thing changed, LGTM so:
>
>Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
>> ---
>>  mm/page_idle.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/page_idle.c b/mm/page_idle.c
>> index 9c67cbac2965..29ee18f0e8ce 100644
>> --- a/mm/page_idle.c
>> +++ b/mm/page_idle.c
>> @@ -40,9 +40,9 @@ static struct folio *page_idle_get_folio(unsigned long pfn)
>>  		return NULL;
>>
>>  	folio = page_folio(page);
>> -	if (!folio_test_lru(folio) || !folio_try_get(folio))
>
>I guess this was meant as a racey check...
>
>> +	if (!folio_try_get(folio))
>>  		return NULL;
>> -	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
>> +	if (unlikely(page_folio(page) != folio) || !folio_test_lru(folio)) {
>
>As above, not sure why you're changing this? Seems unrelated, I'd just keep
>it as it was.
>
>>  		folio_put(folio);
>>  		folio = NULL;
>>  	}
>> --
>> 2.34.1
>>
>
>Cheers, Lorenzo

Re: [PATCH] mm/page_idle: call folio_test_lru() after folio_get()

[Lorenzo Stoakes (ARM)]

Nooooo :) this is not how you send patches.

Also you should _reply_ to review comments, not just send another patch with no
reply. Comms is king.

Also - don't respin so quick. Wait a day.

Then send a v2, NOT in reply to anything.

Really best way is to use b4, docs at
https://b4.docs.kernel.org/en/latest/contributor/prep.html and etc.

But you can also do something like:

git format-patch -v2 HEAD~1
scripts/checkpatch.pl <patch filename>
scripts/get_maintainer.pl <patch filename>
git send-email --to="(andrew)" --cc="<list from get maintainers>" <patch filename>


On Wed, Jul 22, 2026 at 07:20:20PM +0800, jiale yao wrote:
> page_idle_get_folio() speculatively calls folio_test_lru() before
> folio_try_get(). The folio can get freed and reallocated to a tail page
> in the meantime. In that case, VM_BUG_ON_PGFLAGS() in
> const_folio_flags() can be triggered. Remove the speculative call.
>
> This is a sibling-path bug: damon_get_folio() was copied from this
> function with the same flawed pattern. Commit d6b8b02a27b3
> ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed
> damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN
> (strict mode) confirms the data race on the folio flags:
>
>   BUG: KCSAN: data-race in ... / percpu_counter_add_batch
>     page_idle_get_folio+0x7a/0x2d0
>     page_idle_bitmap_read+0xc9/0x220
>
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

this should really be backported, so good to find the right commit to use as a Fixes: here.

Also then add Cc: <stable@vger.kernel.org> so it gets send for backporting.

But _please_ wait a day before sending the v2 :)

> ---

Also in the v2 put a list of changes here (will not be included in commit msg)
with links to previous versions on lore.

>  mm/page_idle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_idle.c b/mm/page_idle.c
> index 9c67cbac2965..b6c26a0ba3d5 100644
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -40,7 +40,7 @@ static struct folio *page_idle_get_folio(unsigned long pfn)
>  		return NULL;
>
>  	folio = page_folio(page);
> -	if (!folio_test_lru(folio) || !folio_try_get(folio))
> +	if (!folio_try_get(folio))
>  		return NULL;
>  	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
>  		folio_put(folio);
> --
> 2.34.1
>
>
>
>
>
> At 2026-07-22 17:56:13, "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:
> >On Wed, Jul 22, 2026 at 05:26:42PM +0800, Jiale Yao wrote:
> >> page_idle_get_folio() speculatively calls folio_test_lru() before
> >> folio_try_get(). The folio can get freed and reallocated to a tail page
> >> in the meantime. In that case, VM_BUG_ON_PGFLAGS() in
> >> const_folio_flags() can be triggered. Remove the speculative call.
> >>
> >> Also mark the folio_test_lru() check right after folio_try_get() success
> >> as no more unlikely.
> >
> >Slightly strange wording but not sure why you're doing that? It is
> >generally unlikely a given folio will be !LRU right?
> >
> >>
> >> This is a sibling-path bug: damon_get_folio() was copied from this
> >> function with the same flawed pattern. Commit d6b8b02a27b3
> >> ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed
> >> damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN
> >> (strict mode) confirms the data race on the folio flags:
> >>
> >>   BUG: KCSAN: data-race in ... / percpu_counter_add_batch
> >>     page_idle_get_folio+0x7a/0x2d0
> >>     page_idle_bitmap_read+0xc9/0x220
> >>
> >> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> >
> >Yeah generally this seems obviously correct (TM), if we can't be sure a
> >folio is kept around any other way to the extent we're doing
> >folio_try_get() we should gate any actual interactions with the folio on
> >succeeding the get first...!
> >
> >With the unlikely thing changed, LGTM so:
> >
> >Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> >
> >> ---
> >>  mm/page_idle.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/page_idle.c b/mm/page_idle.c
> >> index 9c67cbac2965..29ee18f0e8ce 100644
> >> --- a/mm/page_idle.c
> >> +++ b/mm/page_idle.c
> >> @@ -40,9 +40,9 @@ static struct folio *page_idle_get_folio(unsigned long pfn)
> >>  		return NULL;
> >>
> >>  	folio = page_folio(page);
> >> -	if (!folio_test_lru(folio) || !folio_try_get(folio))
> >
> >I guess this was meant as a racey check...
> >
> >> +	if (!folio_try_get(folio))
> >>  		return NULL;
> >> -	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
> >> +	if (unlikely(page_folio(page) != folio) || !folio_test_lru(folio)) {
> >
> >As above, not sure why you're changing this? Seems unrelated, I'd just keep
> >it as it was.
> >
> >>  		folio_put(folio);
> >>  		folio = NULL;
> >>  	}
> >> --
> >> 2.34.1
> >>
> >
> >Cheers, Lorenzo

Thanks, Lorenzo

Re: [PATCH] mm/page_idle: call folio_test_lru() after folio_get()

[SJ Park]

On Wed, 22 Jul 2026 13:50:05 +0100 "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:

> Nooooo :) this is not how you send patches.
> 
> Also you should _reply_ to review comments, not just send another patch with no
> reply. Comms is king.
> 
> Also - don't respin so quick. Wait a day.
> 
> Then send a v2, NOT in reply to anything.
> 
> Really best way is to use b4, docs at
> https://b4.docs.kernel.org/en/latest/contributor/prep.html and etc.
> 
> But you can also do something like:
> 
> git format-patch -v2 HEAD~1
> scripts/checkpatch.pl <patch filename>
> scripts/get_maintainer.pl <patch filename>
> git send-email --to="(andrew)" --cc="<list from get maintainers>" <patch filename>
> 
> 
> On Wed, Jul 22, 2026 at 07:20:20PM +0800, jiale yao wrote:
> > page_idle_get_folio() speculatively calls folio_test_lru() before
> > folio_try_get(). The folio can get freed and reallocated to a tail page
> > in the meantime. In that case, VM_BUG_ON_PGFLAGS() in
> > const_folio_flags() can be triggered. Remove the speculative call.
> >
> > This is a sibling-path bug: damon_get_folio() was copied from this
> > function with the same flawed pattern. Commit d6b8b02a27b3
> > ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed
> > damon_get_folio(), but page_idle_get_folio() was left unfixed.

I was actually thinking I should also do this, but I was again failed at
managing my memory.  Thank you for doing this, Jiale!

> > KCSAN
> > (strict mode) confirms the data race on the folio flags:
> >
> >   BUG: KCSAN: data-race in ... / percpu_counter_add_batch
> >     page_idle_get_folio+0x7a/0x2d0
> >     page_idle_bitmap_read+0xc9/0x220
> >
> > Signed-off-by: Jiale Yao <yaojiale02@163.com>
> > Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> 
> this should really be backported, so good to find the right commit to use as a Fixes: here.
> 
> Also then add Cc: <stable@vger.kernel.org> so it gets send for backporting.
> 
> But _please_ wait a day before sending the v2 :)
> 
> > ---
> 
> Also in the v2 put a list of changes here (will not be included in commit msg)
> with links to previous versions on lore.

Assuming all the above nice suggestions from Lorenzo are accepted,

Reviewed-by: SJ Park <sj@kernel.org>


Thanks,
SJ

[...]

Re: [PATCH] mm/page_idle: call folio_test_lru() after folio_get()

[SJ Park]

On Wed, 22 Jul 2026 10:56:13 +0100 "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:

> On Wed, Jul 22, 2026 at 05:26:42PM +0800, Jiale Yao wrote:
> > page_idle_get_folio() speculatively calls folio_test_lru() before
> > folio_try_get(). The folio can get freed and reallocated to a tail page
> > in the meantime. In that case, VM_BUG_ON_PGFLAGS() in
> > const_folio_flags() can be triggered. Remove the speculative call.
> >
> > Also mark the folio_test_lru() check right after folio_try_get() success
> > as no more unlikely.
> 
> Slightly strange wording but not sure why you're doing that? It is
> generally unlikely a given folio will be !LRU right?
> 
> >
> > This is a sibling-path bug: damon_get_folio() was copied from this
> > function with the same flawed pattern. Commit d6b8b02a27b3
> > ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed
> > damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN
> > (strict mode) confirms the data race on the folio flags:
> >
> >   BUG: KCSAN: data-race in ... / percpu_counter_add_batch
> >     page_idle_get_folio+0x7a/0x2d0
> >     page_idle_bitmap_read+0xc9/0x220
> >
> > Signed-off-by: Jiale Yao <yaojiale02@163.com>
> 
> Yeah generally this seems obviously correct (TM), if we can't be sure a
> folio is kept around any other way to the extent we're doing
> folio_try_get() we should gate any actual interactions with the folio on
> succeeding the get first...!
> 
> With the unlikely thing changed, LGTM so:
> 
> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> 
> > ---
> >  mm/page_idle.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/page_idle.c b/mm/page_idle.c
> > index 9c67cbac2965..29ee18f0e8ce 100644
> > --- a/mm/page_idle.c
> > +++ b/mm/page_idle.c
> > @@ -40,9 +40,9 @@ static struct folio *page_idle_get_folio(unsigned long pfn)
> >  		return NULL;
> >
> >  	folio = page_folio(page);
> > -	if (!folio_test_lru(folio) || !folio_try_get(folio))
> 
> I guess this was meant as a racey check...
> 
> > +	if (!folio_try_get(folio))
> >  		return NULL;
> > -	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
> > +	if (unlikely(page_folio(page) != folio) || !folio_test_lru(folio)) {
> 
> As above, not sure why you're changing this? Seems unrelated, I'd just keep
> it as it was.

Maybe Jiale followed the pattern in DAMON side fix (commit d6b8b02a27b3).  I
was making a similar change for the below reason.  The second test_lru() was
unlikely because it is the second test.  Now it became the first test, so it is
"less" unlikely than before.  I don't expect this makes some real change,
though.  Keeping this as is looks fine to me.


Thanks,
SJ

[...]