Re: [PATCH] mm/mglru: check need_resched() in fill/drain_evictable()

[Hui Zhu <hui.zhu@linux.dev>]

Hi Michal and Kairui,

2026/11/12 16:09, Michal Hocko:
> On Thu 12-11-26 16:03:54, Kairui Song wrote:
>> On Thu, Nov 12, 2026 at 03:58:27PM +0800, Hui Zhu wrote:
>>> From: Hui Zhu <zhuhui@kylinos.cn>
>>>
>>> lru_gen_change_state() migrates folios in batches of up to
>>> MAX_LRU_BATCH (4096) folios under lruvec_lock_irq().  A full batch
>>> keeps local interrupts off for up to about a millisecond, and CPUs
>>> contending for the same lru_lock spin with interrupts disabled for
>>> that long.
>>>
>>> The caller already reschedules between batches, but a batch itself
>>> only ends after 4096 folios.  Stop it early when a resched is
>>> pending; the caller restarts the migration where it left off.  A
>>> remote resched_curr() sets TIF_NEED_RESCHED directly in the target's
>>> thread_info, so the flag can still be set with interrupts off.
>>>
>>> This follows walk_pud_range(), which already pairs need_resched()
>>> with a MAX_LRU_BATCH cap, and commit 6e9be217a3ce ("mm/mglru: use a
>>> smaller batch for reclaim").
>>>
>>> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
>>> ---
>>>   mm/vmscan.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index 6a1af9b92349..47f2cb25eb46 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -5533,7 +5533,8 @@ static bool fill_evictable(struct lruvec *lruvec)
>>>   			success = lru_gen_add_folio(lruvec, folio, !active);
>>>   			VM_WARN_ON_ONCE(!success);
>>>   
>>> -			if (!--remaining)
>>> +			/* stop early to shorten the irq-off section */
>>> +			if (!--remaining || need_resched())
>> Hi Hui,
>>
>> I've been thinking about a different design, what if we just
>> delete the `--remaining` check completely, and check
>> spin_is_contended on the lruvec and need_resched()?
>>
>> In the optimal case we don't need to keep lock/unlock the lruvec
>> at all so this is faster. And it still avoids lock contention or
>> blocked schedule, and it might be even more responsive as we are
>> release the lock earlier on contention.
I like the spin_is_contended() idea and will add it.  This file
already uses it (lru_gen_look_around() bails out if the PTL is
contended), and security/keys/gc.c uses exactly this combination in a
long list walk under a spinlock.
> Wouldn't this result in too much lock bouncing in the end? Would it make
> sense to reduce the batch instead?
>
This worry is fair.  Under sustained contention each yield costs an
unlock, a requeue and a restart (microseconds), while one folio move
costs a few hundred nanoseconds, so the migration could slow down by
tens of times, with cgroup_lock and cpus_read_lock held for all of it.
Requiring a minimum of work per hold bounds this: only honor
spin_is_contended() after MIN_LRU_BATCH folios in the current hold.
Each hold then moves at least ~64 folios (~15us), and a waiter still
waits only ~15us instead of the ~1ms of a full MAX_LRU_BATCH batch.

I'd still keep the batch cap itself as a backstop, though.  Both
triggers are event driven: with interrupts off the local tick cannot
set TIF_NEED_RESCHED, so it takes a remote wakeup targeting this CPU,
and spin_is_contended() takes somebody grabbing this lru_lock.  On a
quiet system neither may ever fire, and one batch becomes the whole
lruvec: 100GB of 4K folios is ~25M of them, i.e. seconds of irq-off
inside a single rcu_read_lock() section, enough for an RCU stall. And
toggling MGLRU on a quiet machine is a common case.

Reducing the batch alone would also work and is simpler, but it pays
the unlock/relock round trip even when nobody is waiting, and on a
loaded machine the unconditional cond_resched() every small batch
stretches the whole switch more than yielding only when somebody
actually wants the lock or the CPU.

So how about all three combined, like jbd2_log_do_checkpoint() does
(batch cap || need_resched() || spin_needbreak()):

     if (!--remaining || need_resched() ||
         (moved >= MIN_LRU_BATCH && spin_is_contended(&lruvec->lru_lock)))
         return false;

Should I send a v2 with this?

Best,
Hui