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

[Barry Song]

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:

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