Re: [PATCH v2 05/26] mm/fbatch: lru_add_del_folio()+folio_add_lru() after clear_lru()

[Hugh Dickins <hughd@google.com>]

On Wed, 9 Sep 2026, Vlastimil Babka (SUSE) wrote:
> On 9/9/26 11:51, Hugh Dickins wrote:
> > Most callers of folio_test_clear_lru() then proceed to remove the folio
> > from its lru, and add it back at the end when they're done (if still in
> > use). But isolate_migratepages_block() and check_move_unevictable_pages()
> > sometimes decide against, and release immediately with a folio_set_lru().
> > 
> > Which usually works fine: but there's now a small chance that while they
> > held the folio with lru bit cleared, an lru_add fbatch drain came along,
> > and had to skip that folio because its lru bit was transiently cleared
> > (previously, the lru_add fbatch drain relied on finding lru bit never yet
> > set). This risks leaving that folio off lru, unreclaimable until freed.
> 
> So this makes the previous patch a somewhat bisection hazard? I guess it's
> acceptable given it's not fatal.

Not what I would call a bisection hazard. Yes, the preceding patch
is not perfect, but more reviewable that way, and then come corrections
to edge cases best considered by themselves. Nobody bisecting unrelated
issues would get held up by this gap, and it won't crash any bisections.

> 
> > Fix such cases by trying lru_add_del_folio() (which only takes action and
> > returns true if the folio was on an lru_add fbatch), then folio_add_lru()
> 
> Oh ok, that's one detail I didn't realize on the previous patch, and
> explains the name of the function. But it's still IMHO confusing.
> 
> > if it succeeded: invalidating the old fbatch slot, appending in a new one.
> > 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> 
> In general, LGTM.
> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>

Thanks.

> 
> Nit below:

> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index f11491ee9ed5..4e8d5cc34f07 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -8093,17 +8093,19 @@ void check_move_unevictable_folios(struct folio_batch *fbatch)
> >  			folio_clear_unevictable(folio);
> >  			lruvec_add_folio(lruvec, folio);
> >  			pgrescued += nr_pages;
> > +		} else if (lru_add_del_folio(folio)) {
> > +			lruvec_unlock_irq(lruvec);
> > +			folio_add_lru(folio);
> > +			lruvec = NULL;
> >  		}
> > -		folio_set_lru(folio);
> > +		if (lruvec)
> > +			folio_set_lru(folio);
> >  	}
> >  
> > -	if (lruvec) {
> > -		__count_vm_events(UNEVICTABLE_PGRESCUED, pgrescued);
> > -		__count_vm_events(UNEVICTABLE_PGSCANNED, pgscanned);
> > +	if (lruvec)
> >  		lruvec_unlock_irq(lruvec);
> > -	} else if (pgscanned) {
> > -		count_vm_events(UNEVICTABLE_PGSCANNED, pgscanned);
> > -	}
> > +	count_vm_events(UNEVICTABLE_PGRESCUED, pgrescued);
> > +	count_vm_events(UNEVICTABLE_PGSCANNED, pgscanned);
> 
> AFAIU this is done because we can no longer rule out that !lruvec means
> pgrescued is 0.
> But we can still distinguish the cheaper __count_vm_events vs
> count_vm_events? Probably all the same on x86, but I hear on arm64 this_cpu*
> ops have a cost worth proposing rather elaborate schemes to deal with...

Yes, it was just looking a bit baroque to still be deciding whether to
use the __count or the count there. Could be done of course, and with
"if (pgrescued)" and "if (pgscanned)"; but I haven't noticed anywhere
else in the source where we go to such lengths to use __count versus
count, and I don't think this is on anyone's hotpath (IIRC this is
just SHM_UNLOCK).

Now you've got me worried, no, fractionally worried, about Shakeel's
recent __count to count fix to NR_MLOCK.

I am much more familiar with x86, and have noticed the recent tussles
over improving arm64 this_cpus, so that confirms you're right; but I'd
look for juicier low-hanging fruit than this, if we're going to
embark on an "if (x) __count() else count()" spree.

Hugh