Re: [RFC PATCH v3 3/6] mm/mglru: improve readability of isolate_folios()

[Baolin Wang]

On 8/3/26 12:46 PM, Barry Song wrote:
> On Mon, Aug 3, 2026 at 11:22 AM Baolin Wang
> <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
> > On 7/31/26 4:38 PM, Barry Song (Xiaomi) 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.
> > >
> > > Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>
> > > Co-developed-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > > Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > > ---
> >
> > Good cleanup. Some comments below.
> >
> > >    mm/vmscan.c | 47 +++++++++++++++++++++++++++--------------------
> > >    1 file changed, 27 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 31947fa60f18..0038f33aa318 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -4828,35 +4828,42 @@ 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 single_type = is_single_type_reclaim(swappiness);
> > >        int type = get_type_to_scan(lruvec, swappiness);
> > > +     int total_scanned = 0, scanned, tier;
> > > +     bool tried = false;
> >
> > The tried' is a bit confusing. How about 'type_fallback_allowed'? Or
> > other more readable variable name?
> >
> > In addition, the 'single_type' variable can be removed, and just
> > initialize 'type_fallback_allowed' directly:
> >
> > bool type_fallback_allowed = !is_single_type_reclaim(swappiness);
>
>
> Yep. It seems we can simply use type_fallback_allowed and drop
> tried entirely:

Thanks. That's what I meant:)

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 1ffd96c87bc2..358388b49a70 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4840,10 +4840,9 @@ static int isolate_folios(unsigned long
> nr_to_scan, struct lruvec *lruvec,
>                            struct list_head *list, int *isolated,
>                            int *isolate_type, int *isolate_scanned)
>   {
> -       bool single_type = is_single_type_reclaim(swappiness);
> +       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);
> @@ -4861,9 +4860,9 @@ 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 (!tried && !scanned && !single_type) {
> +       if (type_fallback_allowed && !scanned) {
>                  type = !type;
> -               tried = true;
> +               type_fallback_allowed = false;
>                  goto retry;
>          }