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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 11/13/26 11:08 AM, Hui Zhu wrote:
> 
> 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.

If so, why not just set:

int remaining = MIN_LRU_BATCH;
?

> 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.

fill_evictable()/drain_evictable() are really cold paths, I really don't 
see any practical impact from this. At least we (Alibaba) won't be 
dynamically switching MGLRU state in practice.