Re: [PATCH v2 05/12] mm/mglru: scan and count the exact number of folios

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

On 3/31/26 5:01 PM, Kairui Song wrote:
> On Tue, Mar 31, 2026 at 04:04:30PM +0800, Baolin Wang wrote:
>>
>>
>> On 3/29/26 3:52 AM, Kairui Song via B4 Relay wrote:
>>> From: Kairui Song <kasong@tencent.com>
>>>
>>> Make the scan helpers return the exact number of folios being scanned
>>> or isolated. Since the reclaim loop now has a natural scan budget that
>>> controls the scan progress, returning the scan number directly should
>>> make the scan more accurate and easier to follow.
>>>
>>> The number of scanned folios for each iteration is always positive and
>>> larger than 0, unless the reclaim must stop for a forced aging, so
>>> there is no more need for any special handling when there is no
>>> progress made:
>>>
>>> - `return isolated || !remaining ? scanned : 0` in scan_folios: both
>>>     the function and the call now just return the exact scan count,
>>>     combined with the scan budget introduced in the previous commit to
>>>     avoid livelock or under scan.
>>
>> Make sense to me.
>>
>>>
>>> - `scanned += try_to_inc_min_seq` in evict_folios: adding a bool as a
>>>     scan count was kind of confusing and no longer needed too, as scan
>>>     number will never be zero even if none of the folio in oldest
>>>     generation is isolated.
>>
>> Yes, agree.
>>
>>>
>>> - `evictable_min_seq + MIN_NR_GENS > max_seq` guard in evict_folios:
>>>     the per-type get_nr_gens == MIN_NR_GENS check in scan_folios
>>>     naturally returns 0 when only two gens remain and breaks the loop.
>>>
>>> Also move try_to_inc_min_seq before isolate_folios, so that any empty
>>> gens created by external folio freeing are also skipped.
>>
>> This part is somewhat confusing. You probably mean the case where the list
>> of that gen becomes empty via isolate_folio(), right?
>>
>> If that's the case, the original logic would remove the empty gens produced
>> by isolate_folio() after calling try_to_inc_min_seq().
>>
>> However, with your changes, this removal won't happen until the next
>> eviction. Does this provide any additional benefits? Or could you describe
>> how this change impacts your testing?
> 
> Hi Baolin, thanks for the review.
> 
> Yeah, I also notices this issue after sending this while doing more
> self review.
> 
> So I did some test with the patch below:
> 
> static bool inc_max_seq(struct lruvec *lruvec, unsigned long seq, int swappiness)
> @@ -4818,11 +4814,15 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>   
>          lruvec_lock_irq(lruvec);
>   
> +       /* In case folio deletion created empty gen, flush them */
>          try_to_inc_min_seq(lruvec, swappiness);
>   
>          scanned = isolate_folios(nr_to_scan, lruvec, sc, swappiness,
>                                   &list, &isolated, &type, &type_scanned);
>   
> +       /* Isolation might created empty gen, flush them */
> +       try_to_inc_min_seq(lruvec, swappiness);
> +
>          lruvec_unlock_irq(lruvec);
>   
>          if (list_empty(&list))
> 
> The return value of try_to_inc_min_seq can also be dropped
> since it's no longer used, and the function call should be cheap.
> 
> After system time of build kernel using 3G memory and make -j96
> with ZRAM as swap, system time in seconds average of 12 test run each:
> 
> mm-new:
> 9136.055833
> 
> After V2:
> 8819.932222
> 
> After V2, with above patch:
> 8783.944444
> 
> After V2, without above patch but move try_to_inc_min_seq
> back to after isolate_folios:
> 8807.874444
> 
> This series is looking good, this inc_min change seems trivial
> but in theory it does have have real effect.
> 
> - Moving the try_to_inc_min_seq after isolate_folios may result in a
> wasted isolate_folios call and early abort of reclaim loop if there
> is a stalled oldest gen created by folio deletion.

Indeed.

> - Moving the try_to_inc_min_seq before isolate_folios may leave a
> empty gen after isolation. Usually it's fine because next eviction
> will still reclaim them. But before next eviction, during that period,
> new file folios could be added the oldest gen and get reclaim too
> early. That looks a real problem.
> 
> This maybe trivial since MGLRU itself also may suffer the same
> problem when the oldest gen is just too short, that's a much more
> common case (For this short oldest gen issue we can solve later).
> 
> - Having try_to_inc_min_seq both before and after isolate_folios
> seems the best choice here and somehow matches the benchmark
> result above, very close to the noise level though.
> 
> Well I only tested one cases, the cover letter described a
> larger matrix, still all good with this series and I'm not
> 100% sure how this particular change effects them, I guess
> it's still trivial.
> 
> The try_to_inc_min_seq call should be cheap enough since it's
> called only for one batch of 64 folios, and it's only reading
> a few lists for the non inc path.
> 
> How do you think that we just call it twice here?

Sounds reasonable to me.

I'm not sure if we need to split out a new patch with adding above 
message, as this patch mainly focuses on optimizing the number of folios 
being scanned.