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

[Baoquan He]

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;
	...
}

2, for swappiness (0, 200), the behavious is minor changed.

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:

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;
              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
same-type retry, and cannot produce a third scan. Just personal opinion.

> +	}
>  
>  	return total_scanned;
>  }
> -- 
> 2.34.1
>