Re: [PATCH v5 05/14] mm/mglru: scan and count the exact number of folios

[Kairui Song]

On Fri, Apr 17, 2026 at 1:39 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
> On Thu, Apr 16, 2026 at 3:03 PM Barry Song <baohua@xxxxxxxxxx> wrote:
> >
> > On Mon, Apr 13, 2026 at 12:48 AM Kairui Song via B4 Relay
> > <devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
> > >
> > >  static bool inc_max_seq(struct lruvec *lruvec, unsigned long seq, int swappiness)
> > > @@ -4686,7 +4681,7 @@ static bool isolate_folio(struct lruvec *lruvec, struct folio *folio, struct sca
> > >
> > >  static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > >                        struct scan_control *sc, int type, int tier,
> > > -                      struct list_head *list)
> > > +                      struct list_head *list, int *isolatedp)
> > >  {
> > >         int i;
> > >         int gen;
> > > @@ -4756,11 +4751,9 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > >                                 type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON);
> > >         if (type == LRU_GEN_FILE)
> > >                 sc->nr.file_taken += isolated;
> > > -       /*
> > > -        * There might not be eligible folios due to reclaim_idx. Check the
> > > -        * remaining to prevent livelock if it's not making progress.
> > > -        */
> > > -       return isolated || !remaining ? scanned : 0;
> > > +
> > > +       *isolatedp = isolated;
> > > +       return scanned;
> > >  }
> > >
> > >  static int get_tier_idx(struct lruvec *lruvec, int type)
> > > @@ -4804,33 +4797,36 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)
> > >
> > >  static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > >                           struct scan_control *sc, int swappiness,
> > > -                         int *type_scanned, struct list_head *list)
> > > +                         struct list_head *list, int *isolated,
> > > +                         int *isolate_type, int *isolate_scanned)
> > >  {
> > >         int i;
> > > +       int scanned = 0;
> >
> > I would prefer to rename this to total_scanned.
>
> Good suggestion.
>
> >
> > >         int type = get_type_to_scan(lruvec, swappiness);
> > >
> > >         for_each_evictable_type(i, swappiness) {
> > > -               int scanned;
> > > +               int type_scan;
> >
> > And then we keep this as "scanned".
> >
> > >                 int tier = get_tier_idx(lruvec, type);
> > >
> > > -               *type_scanned = type;
> > > +               type_scan = scan_folios(nr_to_scan, lruvec, sc,
> > > +                                       type, tier, list, isolated);
> > >
> > > -               scanned = scan_folios(nr_to_scan, lruvec, sc, type, tier, list);
> > > -               if (scanned)
> > > -                       return scanned;
> > > +               scanned += type_scan;
> > > +               if (*isolated) {
> > > +                       *isolate_type = type;
> > > +                       *isolate_scanned = type_scan;
> > > +                       break;
> > > +               }
> > >
> > >                 type = !type;
> > >         }
> > >
> > > -       return 0;
> > > +       return scanned;
> >
> > Then
> > return total_scanned;
> >
> > >  }
> > >
> > >  static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > >                         struct scan_control *sc, int swappiness)
> > >  {
> > > -       int type;
> > > -       int scanned;
> > > -       int reclaimed;
> > >         LIST_HEAD(list);
> > >         LIST_HEAD(clean);
> > >         struct folio *folio;
> > > @@ -4838,19 +4834,23 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > >         enum node_stat_item item;
> > >         struct reclaim_stat stat;
> > >         struct lru_gen_mm_walk *walk;
> > > +       int scanned, reclaimed;
> > > +       int isolated = 0, type, type_scanned;
> > >         bool skip_retry = false;
> > > -       struct lru_gen_folio *lrugen = &lruvec->lrugen;
> > >         struct mem_cgroup *memcg = lruvec_memcg(lruvec);
> > >         struct pglist_data *pgdat = lruvec_pgdat(lruvec);
> > >
> > >         lruvec_lock_irq(lruvec);
> > >
> > > -       scanned = isolate_folios(nr_to_scan, lruvec, sc, swappiness, &type, &list);
> > > +       /* In case folio deletion left empty old gens, flush them */
> > > +       try_to_inc_min_seq(lruvec, swappiness);
> > >
> > > -       scanned += try_to_inc_min_seq(lruvec, swappiness);
> > > +       scanned = isolate_folios(nr_to_scan, lruvec, sc, swappiness,
> > > +                                &list, &isolated, &type, &type_scanned);
> > >
> > > -       if (evictable_min_seq(lrugen->min_seq, swappiness) + MIN_NR_GENS > lrugen->max_seq)
> > > -               scanned = 0;
> > > +       /* Isolation might create empty gen, flush them */
> > > +       if (scanned)
> >
> > scanned is not equal to isolated, right?
> > Somehow, I feel the comment does not match the if (scanned).
> > I assume sort_folio() could also create empty gen?
>
> Yeah, you are right, it should be isolated. Usually one batch scan
> always isolates a few folios so I didn't observe much difference in
> testing, but I should definitely fix this. Thanks!

On second thought - it should be "scanned", not "isolated". MGLRU
might move or protect folios to the second oldest gen without
isolating them, we have scan > 0 but isolated == 0, while an empty gen
might be created. Ideally isolate_folios can detect this internally,
which might require some other refactor first. So let's keep it
unchanged here. I'll update the comment about that.