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

[Barry Song]

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?

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?

Thanks
Barry