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

[SJ Park <sj@kernel.org>]

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

[...]