Re: [PATCH v2 03/12] mm/mglru: relocate the LRU scan batch limit to callers

[Barry Song]

On Mon, Mar 30, 2026 at 4:15 PM Baolin Wang
<baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> On 3/29/26 3:52 AM, Kairui Song via B4 Relay wrote:
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Same as active / inactive LRU, MGLRU isolates and scans folios in
> > batches.  The batch split is done hidden deep in the helper, which
> > makes the code harder to follow.  The helper's arguments are also
> > confusing since callers usually request more folios than the batch
> > size, so the helper almost never processes the full requested amount.
> >
> > Move the batch splitting into the top loop to make it cleaner, there
> > should be no behavior change.
> >
> > Reviewed-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > ---
>
> Some nits as follows, otherwise LGTM.
> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>

With the same nits addressed,

Reviewed-by: Barry Song <baohua@xxxxxxxxxx>

>
> >   mm/vmscan.c | 16 +++++++++-------
> >   1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index f336f89a2de6..963362523782 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -4695,10 +4695,10 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> >       int scanned = 0;
> >       int isolated = 0;
> >       int skipped = 0;
> > -     int scan_batch = min(nr_to_scan, MAX_LRU_BATCH);
> > -     int remaining = scan_batch;
> > +     unsigned long remaining = nr_to_scan;
> >       struct lru_gen_folio *lrugen = &lruvec->lrugen;
> >
> > +     VM_WARN_ON_ONCE(nr_to_scan > MAX_LRU_BATCH);
> >       VM_WARN_ON_ONCE(!list_empty(list));
> >
> >       if (get_nr_gens(lruvec, type) == MIN_NR_GENS)
> > @@ -4751,7 +4751,7 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> >       mod_lruvec_state(lruvec, item, isolated);
> >       mod_lruvec_state(lruvec, PGREFILL, sorted);
> >       mod_lruvec_state(lruvec, PGSCAN_ANON + type, isolated);
> > -     trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, scan_batch,
> > +     trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan,
> >                               scanned, skipped, isolated,
> >                               type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON);
> >       if (type == LRU_GEN_FILE)
> > @@ -4987,7 +4987,7 @@ static bool should_abort_scan(struct lruvec *lruvec, struct scan_control *sc)
> >
> >   static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
> >   {
> > -     long nr_to_scan;
> > +     long nr_batch, nr_to_scan;
>
> Nit: Since evict_folios() expects an unsgined long, why not define
> 'unsigned long nr_batch'?

I guess the confusion comes from nr_to_scan being a long
rather than an unsigned long. This is the only place where
nr_to_scan is defined as a long.

I think we should clean up get_nr_to_scan(). Right now, it
clearly returns more than it should, uses -1 to indicate
something else, and also calls try_to_inc_max_seq(), which
is not part of nr_to_scan.

That might be better addressed in a separate patch.

Thanks
Barry