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

[jiale yao]

Hi Lorenzo,

Thanks, this is very helpful for a newbie like me.

For the unlikely() part, I followed the approach in this link:
https://lore.kernel.org/20260525162256.8317-1-sj@xxxxxxxxxx

Thanks,
Jiale


At 2026-07-22 20:50:05, "Lorenzo Stoakes (ARM)" <ljs@xxxxxxxxxx> 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. 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@xxxxxxx>
>> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
>
>this should really be backported, so good to find the right commit to use as a Fixes: here.
>
>Also then add Cc: <stable@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxx>
>> >
>> >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@xxxxxxxxxx>
>> >
>> >> ---
>> >>  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