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

[Baolin Wang]

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