linux-kernel - Re: [PATCH] mm/mglru: Stop try_to_inc_min_seq() if the oldest generation LRU lists are not empty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJj2-QGHLRqY4mPyAPg2eT+y+4yNfNb__nON5ndkY8WG0UmKVQ@mail.gmail.com>
Date: Tue, 1 Jul 2025 17:31:13 -0700
From: Yuanchu Xie <yuanchu@...gle.com>
To: Hao Jia <jiahao.kernel@...il.com>
Cc: akpm@...ux-foundation.org, hannes@...xchg.org, yuzhao@...gle.com, 
	kinseyho@...gle.com, david@...hat.com, mhocko@...nel.org, 
	zhengqi.arch@...edance.com, shakeel.butt@...ux.dev, 
	lorenzo.stoakes@...cle.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Hao Jia <jiahao1@...iang.com>
Subject: Re: [PATCH] mm/mglru: Stop try_to_inc_min_seq() if the oldest
 generation LRU lists are not empty

On Mon, Jun 30, 2025 at 1:06 AM Hao Jia <jiahao.kernel@...il.com> wrote:
>
> From: Hao Jia <jiahao1@...iang.com>
>
> In try_to_inc_min_seq(), if the oldest generation of LRU lists
> (anonymous and file) are not empty. Then we should return directly
> to avoid unnecessary subsequent overhead.
>
> Corollary: If the lrugen->folios[gen][type][zone] lists of both
> anonymous and file are not empty, try_to_inc_min_seq() will fail.
>
> Proof: Taking LRU_GEN_ANON as an example, consider the following two cases:
>
> Case 1: min_seq[LRU_GEN_ANON] <= seq (seq is lrugen->max_seq - MIN_NR_GENS)
>
> Since min_seq[LRU_GEN_ANON] has not increased,
> so min_seq[LRU_GEN_ANON] is still equal to lrugen->min_seq[LRU_GEN_ANON].
> Therefore, in the following judgment:
> min_seq[LRU_GEN_ANON] <= lrugen->min_seq[LRU_GEN_ANON] is always true.
> So, we will not increase the seq of the oldest generation of anonymous,
> and try_to_inc_min_seq() will return false.
>
> case 2: min_seq[LRU_GEN_ANON] > seq (seq is lrugen->max_seq - MIN_NR_GENS)
>
> If min_seq[LRU_GEN_ANON] > seq, that is, lrugen->min_seq[LRU_GEN_ANON] > seq
This part doesn't make sense to me.
The code is as follows:

    /* find the oldest populated generation */
    for_each_evictable_type(type, swappiness) {
        while (min_seq[type] + MIN_NR_GENS <= lrugen->max_seq) {
            gen = lru_gen_from_seq(min_seq[type]);

            for (zone = 0; zone < MAX_NR_ZONES; zone++) {
                if (!list_empty(&lrugen->folios[gen][type][zone]))
                    goto next;
            }

            min_seq[type]++;
        }

Here, it could be that , min_seq[type] > lrugen->max_seq - MIN_NR_GENS
(what you refer to as seq)
However, this is a result of incrementing a copy of
lrugen->min_seq[type] as this piece of code finds the oldest populated
generation.

next:
        ;
    }

> Then min_seq[LRU_GEN_ANON] is assigned seq.
This is not necessarily true, because swappiness can be 0, and the
assignments happen to prevent one LRU type from going more than 1 gen
past the other.
so if `min_seq[LRU_GEN_ANON] > seq && min_seq[LRU_GEN_FILE] == seq` is
true, then min_seq[LRU_GEN_ANON] is not assigned seq.


> Therefore, in the following judgment:
> min_seq[LRU_GEN_ANON] (seq) <= lrugen->min_seq[LRU_GEN_ANON] is always true.
> So, we will not update the oldest generation seq of anonymous,
> and try_to_inc_min_seq() will return false.
>
> It is similar for LRU_GEN_FILE. Therefore, in try_to_inc_min_seq(),
> if the oldest generation LRU lists (anonymous and file) are not empty,
> in other words, min_seq[type] has not increased.
> we can directly return false to avoid unnecessary checking overhead later.
Yeah I don't think this proof holds. If you think it does please
elaborate more and make your assumptions more clear.

>
> Signed-off-by: Hao Jia <jiahao1@...iang.com>
> ---
>  mm/vmscan.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f8dfd2864bbf..3ba63d87563f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3928,6 +3928,7 @@ static bool try_to_inc_min_seq(struct lruvec *lruvec, int swappiness)
>         int gen, type, zone;
>         bool success = false;
>         struct lru_gen_folio *lrugen = &lruvec->lrugen;
> +       int seq_inc_flags[ANON_AND_FILE] = {0};
>         DEFINE_MIN_SEQ(lruvec);
>
>         VM_WARN_ON_ONCE(!seq_is_valid(lruvec));
> @@ -3943,11 +3944,20 @@ static bool try_to_inc_min_seq(struct lruvec *lruvec, int swappiness)
>                         }
>
>                         min_seq[type]++;
> +                       seq_inc_flags[type] = 1;
>                 }
>  next:
>                 ;
>         }
>
> +       /*
> +        * If the oldest generation of LRU lists (anonymous and file)
> +        * are not empty, we can directly return false to avoid unnecessary
> +        * checking overhead later.
> +        */
> +       if (!seq_inc_flags[LRU_GEN_ANON] && !seq_inc_flags[LRU_GEN_FILE])
> +               return success;
> +
>         /* see the comment on lru_gen_folio */
>         if (swappiness && swappiness <= MAX_SWAPPINESS) {
>                 unsigned long seq = lrugen->max_seq - MIN_NR_GENS;
> --
> 2.34.1
>
>
I don't understand what problem this patch tries to solve.

Yuanchu