Re: [PATCH v2] mm/mglru: yield the lru_lock earlier in fill/drain_evictable()

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

2026/11/13 10:50, Baolin Wang :
>
>
> On 11/13/26 10:01 AM, 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, or when the lock has waiters and the current hold has moved
>> at least MIN_LRU_BATCH folios; the caller restarts where it left off.
>> TIF_NEED_RESCHED can still be set with interrupts off: a remote
>> resched_curr() sets it directly in the target's thread_info. The
>> minimum batch avoids lock bouncing under sustained contention, while
>> a waiter now waits ~15us instead of ~1ms.
>>
>> This follows lru_gen_look_around() in this file, security/keys/gc.c
>> and jbd2_log_do_checkpoint(), and commit 6e9be217a3ce ("mm/mglru:
>> use a smaller batch for reclaim").
>>
>> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
>> ---
>> Changlog:
>> v2:
>> Also yield on lock contention after a minimum batch (Kairui Song).
>>
>>   mm/vmscan.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 6a1af9b92349..c1231925159f 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -5533,7 +5533,10 @@ 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() ||
>> +                (MAX_LRU_BATCH - remaining >= MIN_LRU_BATCH &&
>> +                 spin_is_contended(&lruvec->lru_lock)))
>>                   return false;
>
> IIUC, Kairui's point[1] is to just drop the 'MAX_LRU_BATCH - remaining 
> >= MIN_LRU_BATCH' condition, no?
>
> if (!--remaining || need_resched() || 
> spin_is_contended(&lruvec->lru_lock))
>
> [1] 
> https://lore.kernel.org/all/CAMgjq7CUMuNSdCXZ7_kDdo9+0BFvPNHuO=mq=ct_Q5YUyyckvQ@mail.gmail.com/ 
>

Not quite.  [1] was about the extra `moved` counter in the version
discussed on the v1 thread; Kairui was fine with the rest.  Since
`remaining` counts down from MAX_LRU_BATCH and one call is exactly one
lock hold, `MAX_LRU_BATCH - remaining` is the number of folios moved
in the current hold, so the condition is just `moved >= MIN_LRU_BATCH`
without the extra variable.

The minimum-batch test itself is a response to Michal's lock bouncing
concern [2].  Without it, under sustained contention the loop could
yield after every single folio: 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 throughout.  With it, each hold
moves at least MIN_LRU_BATCH (64) folios, i.e. does ~15us of work
before it may hand the lock over, while a waiter still waits only
~15us instead of the ~1ms of a full MAX_LRU_BATCH batch.

[2] https://lore.kernel.org/all/avV0IbvSx4iuv0tF@tiehlicka/

Best,
Hui