Re: [PATCH 1/3] mm/mglru: improve readability of isolate_folios()

[Kairui Song]

On Thu, Aug 20, 2026 at 12:57 PM Barry Song (Xiaomi) <baohua@xxxxxxxxxx> wrote:
>
> From: Ridong Chen <chenridong@xxxxxxxxxx>
>
> The for_each_evictable_type() loop in isolate_folios()
> is misleading: it does not actually iterate over each
> evictable type. Instead, get_type_to_scan() selects the
> type to scan, while the iterator `i` merely bounds the
> number of attempts.
>
> Make the fallback behavior explicit in the code and remove the
> opaque for_each_evictable_type(i, swappiness).
>
> Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>
> Co-developed-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> ---
>  mm/vmscan.c | 46 ++++++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c1404a59523d..d5cc30b667ad 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4833,35 +4833,41 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)
>         return positive_ctrl_err(&sp, &pv);
>  }
>
> +static inline bool is_single_type_reclaim(int swappiness)
> +{
> +       return swappiness == MIN_SWAPPINESS ||
> +              swappiness == SWAPPINESS_ANON_ONLY;
> +}
> +
>  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)
>  {
> -       int i;
> -       int total_scanned = 0;
> +       bool type_fallback_allowed = !is_single_type_reclaim(swappiness);

Hmm, this is the only user of the function and it takes the negative
of the function, will it be better to just revert the conditions and
rename the function?

e.g. is_mixed_reclaim / is_proportional_reclaim, return swappiness !=
MIN_SWAPPINESS && swappiness != SWAPPINESS_ANON_ONLY?

>         int type = get_type_to_scan(lruvec, swappiness);
> +       int total_scanned = 0, scanned, tier;
>
> -       for_each_evictable_type(i, swappiness) {
> -               int scanned;
> -               int tier = get_tier_idx(lruvec, type);
> +retry:
> +       tier = get_tier_idx(lruvec, type);
> +       scanned = scan_folios(nr_to_scan, lruvec, sc,
> +                             type, tier, list, isolated);
>
> -               scanned = scan_folios(nr_to_scan, lruvec, sc,
> -                                     type, tier, list, isolated);
> +       total_scanned += scanned;
> +       if (*isolated) {
> +               *isolate_type = type;
> +               *isolate_scanned = scanned;
> +               return total_scanned;
> +       }
>
> -               total_scanned += scanned;
> -               if (*isolated) {
> -                       *isolate_type = type;
> -                       *isolate_scanned = scanned;
> -                       break;
> -               }
> -               /*
> -                * If scanned > 0 and isolated == 0, avoid falling back to the
> -                * other type, as this type remains sufficient. Falling back
> -                * too readily can disrupt the positive_ctrl_err() bias.
> -                */
> -               if (!scanned)
> -                       type = !type;
> +       /*
> +        * We are running out of the current reclaim type. Fall back to
> +        * the other type if allowed.
> +        */
> +       if (!scanned && type_fallback_allowed) {
> +               type = !type;
> +               type_fallback_allowed = false;
> +               goto retry;
>         }

Looks good to me, the nitpick can be ignored.

Reviewed-by: Kairui Song <kasong@xxxxxxxxxxx>