Re: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()

[Baoquan He]

On 09/02/26 at 05:20pm, Barry Song wrote:
> On Wed, Sep 2, 2026 at 4:07 PM Baoquan He <baoquan.he@xxxxxxxxx> wrote:
> >
> > Hi Barry,
> >
> > On 08/29/26 at 03:42pm, Barry Song (Xiaomi) wrote:
> > > The existing mainline code retries the same type once in a rather
> > > subtle way. `for_each_evictable_type()` may provide one more iteration,
> > > allowing the same type to be retried if we scanned some folios but
> > > failed to isolate any due to protections, promotions, or races. This
> > > patch makes the retry behavior explicit.
> > >
> > > Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > > ---
> > >  mm/vmscan.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 35a233623368..718f59ffc688 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -4852,6 +4852,7 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > >       bool type_fallback_allowed = !is_single_type_reclaim(swappiness);
> > >       int type = get_type_to_scan(lruvec, swappiness);
> > >       int total_scanned = 0, scanned, tier;
> > > +     bool tried = false;
> > >
> > >  retry:
> > >       tier = get_tier_idx(lruvec, type);
> > > @@ -4871,9 +4872,18 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > >        */
> > >       if (!scanned && type_fallback_allowed) {
> > >               type = !type;
> > > +             tried = true;
> > >               type_fallback_allowed = false;
> > >               goto retry;
> > >       }
> > > +     /*
> > > +      * We scanned some folios but failed to isolate any due to promotions,
> > > +      * protections, or races. Retry once to avoid a larger loop.
> > > +      */
> > > +     if (scanned && !tried) {
> > > +             tried = true;
> > > +             goto retry;
> >
> > Seems patch 1 and 2 makes not minor difference than mainline kernel on
> > behaviour.
> >
> > 1, if swappiness is 0 because no swap, it will run two times if
> > (scanned != 0). This is not corner case, but usually seen on some
> > systems w/o swap device. The 2nd no gain run could decrease efficiency.
> >
> > static int get_swappiness(struct lruvec *lruvec, struct scan_control *sc)
> > {
> >         ...
> >
> >         if (!sc->may_swap)
> >                 return 0;
> >         ...
> > }
> 
> Yep. For swappiness 0 and 201, this patch slightly changes the
> behavior, as I mentioned in the cover letter:
> "
>  There is a slight functional change for 0 and 201: with the existing
>  code, there is no chance to retry for these values because
>  `for_each_evictable_type()` only iterates once. After this patch, 0 and
>  201 have behavior that is more consistent with the 1-200 range.
> "
> I did this intentionally, as it makes the behavior more consistent with
> the 1-200 range, where we retry the same type once to avoid having a
> larger outer loop.
> 
> >
> > 2, for swappiness (0, 200), the behavious is minor changed.
> 
> I guess you actually mean swappiness (1, 200)?
> 
> >
> > Mark one scan_folios() result as one of:
> >       iso     *isolated > 0
> >       empty   scanned == 0 && !*isolated
> >       busy    scanned > 0  && !*isolated
> >
> > mainline:  T(busy) -> T(empty)         -> return   (2 scans, no fallback)
> > v2:        T(busy) -> T(empty) -> !T(...)        (a 3rd scan_folios())
> >
> > Maybe we can go like below:
> 
> Yes, you're right. For swappiness (1, 200), I didn't realize there was
> this slight change.
> 
> >
> > static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> >                           struct scan_control *sc, int swappiness,
> >                           struct list_head *list, int *isolated,
> >                           int *isolate_type, int *isolate_scanned)
> >         ...
> >
> >       for (attempt = 0; attempt < 2; attempt++) {
> >               int scanned = scan_folios(nr_to_scan, lruvec, sc, type,
> >                                         get_tier_idx(lruvec, type), list, isolated);
> >
> >               total_scanned += scanned;
> >               if (*isolated) {
> >                       *isolate_type = type;
> >                       *isolate_scanned = scanned;
> >                       return total_scanned;
> >               }
> >               if (attempt)                /* already retried / fell back once */
> >                       break;
> 
> This `if (attempt) break` makes the loop look rather strange,
> especially for a loop with a maximum of 2 iterations, where we break
> when `attempt` reaches 1 :-)
> 
> What about just changing one line?

Hi Barry,

Agreed on the one-line change for the (1, 200) case - I traced it and it
now matches mainline exactly (no extra third scan). I personally prefer
the for (attempt = 0... ) style because I feel that makes logic clearer,
while everybody truly has different code taste, LOL, just a weak opinion. 

For 0/201: my concern is that on no-swap systems (swappiness 0 is
file-only), the same-type retry when the first scan is busy may be a
no-gain run if the file generation is dominated by protected/ineligible
folios - the retry re-scans the same sort results. But if you see a case
where the retry does isolate folios on the second pass for single-type
reclaim, keeping it for consistency is defensible. Do you have such a
case, or should we drop the retry for 0/201?

> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bf2786c7247d..ba7adf36e69f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4939,7 +4939,7 @@ static int isolate_folios(unsigned long
> nr_to_scan, struct lruvec *lruvec,
>          * We are running out of the current reclaim type. Fall back to
>          * the other type if allowed.
>          */
> -       if (!scanned && type_fallback_allowed) {
> +       if (!scanned && !tried && type_fallback_allowed) {
>                 type = !type;
>                 tried = true;
>                 type_fallback_allowed = false;
> 
> >               if (scanned)
> >                       continue;           /* retry the same type once */
> >               if (single_type)
> >                       break;              /* no fallback for 0 / anon-only */
> >               type = !type;               /* empty: fall back to the other type */
> >       }
> >
> >       return total_scanned;
> > }
> >
> > This preserves mainline for 1..200 exactly, keeps the intended 0/201
> 
> I guess you mean removing the same-type retry for swappiness 0/201,
> rather than keeping it?

You are right, it was a slip of the tongue.