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

[Barry Song]

On Mon, Apr 13, 2026 at 12:48 AM Kairui Song via B4 Relay
<devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
>
> From: Kairui Song <kasong@xxxxxxxxxxx>
>
> Make the scan helpers return the exact number of folios being scanned
> or isolated. Since the reclaim loop now has a natural scan budget that
> controls the scan progress, returning the scan number and consume the
> budget should make the scan more accurate and easier to follow.
>
> The number of scanned folios for each iteration is always positive and
> larger than 0, unless the reclaim must stop for a forced aging, so
> there is no more need for any special handling when there is no
> progress made:
>
> - `return isolated || !remaining ? scanned : 0` in scan_folios: both
>   the function and the call now just return the exact scan count,
>   combined with the scan budget introduced in the previous commit to
>   avoid livelock or under scan.
>
> - `scanned += try_to_inc_min_seq` in evict_folios: adding a bool as a
>   scan count was kind of confusing and no longer needed to, as scan
>   number should never be zero as long as there are still evictable
>   gens. We may encounter a empty old gen that return 0 scan count,
>   to avoid that, do a try_to_inc_min_seq before isolation which
>   have slight to none overhead in most cases.
>
> - `evictable_min_seq + MIN_NR_GENS > max_seq` guard in evict_folios:
>   the per-type get_nr_gens == MIN_NR_GENS check in scan_folios
>   naturally returns 0 when only two gens remain and breaks the loop.
>
> Also change try_to_inc_min_seq to return void, as its return value is
> no longer used by any caller. Move the call before isolate_folios so
> that any empty gens created by external folio freeing are flushed, and
> add another call after isolate_folios to also flush empty gens that
> isolation itself may create.
>
> The scan still stops if there are only two gens left as the scan number
> will be zero, this behavior is same as before. This force gen protection
> may get removed or softened later to improve the reclaim a bit more.
>
> Reviewed-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
> Reviewed-by: Chen Ridong <chenridong@xxxxxxxxxxxxxxx>
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> ---
>  mm/vmscan.c | 60 ++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index d4aaaa62056d..e3b68b008376 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3878,10 +3878,9 @@ static bool inc_min_seq(struct lruvec *lruvec, int type, int swappiness)
>         return true;
>  }
>
> -static bool try_to_inc_min_seq(struct lruvec *lruvec, int swappiness)
> +static void try_to_inc_min_seq(struct lruvec *lruvec, int swappiness)
>  {
>         int gen, type, zone;
> -       bool success = false;
>         bool seq_inc_flag = false;
>         struct lru_gen_folio *lrugen = &lruvec->lrugen;
>         DEFINE_MIN_SEQ(lruvec);
> @@ -3907,11 +3906,10 @@ static bool try_to_inc_min_seq(struct lruvec *lruvec, int swappiness)
>
>         /*
>          * If min_seq[type] of both anonymous and file is not increased,
> -        * we can directly return false to avoid unnecessary checking
> -        * overhead later.
> +        * return here to avoid unnecessary checking overhead later.
>          */
>         if (!seq_inc_flag)
> -               return success;
> +               return;
>
>         /* see the comment on lru_gen_folio */
>         if (swappiness && swappiness <= MAX_SWAPPINESS) {
> @@ -3929,10 +3927,7 @@ static bool try_to_inc_min_seq(struct lruvec *lruvec, int swappiness)
>
>                 reset_ctrl_pos(lruvec, type, true);
>                 WRITE_ONCE(lrugen->min_seq[type], min_seq[type]);
> -               success = true;
>         }
> -
> -       return success;
>  }

I like that you removed those "success" checks;
they have caused me to fail many times.

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

>         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?

> +               try_to_inc_min_seq(lruvec, swappiness);

Thanks
Barry