Re: [PATCH v2] mm/mglru: Stop try_to_inc_min_seq() if min_seq[type] has not increased

[Axel Rasmussen <axelrasmussen@google.com>]

On Wed, Jul 2, 2025 at 7:40 PM Hao Jia <jiahao.kernel@gmail.com> wrote:
>
> From: Hao Jia <jiahao1@lixiang.com>
>
> In try_to_inc_min_seq(), if min_seq[type] has not increased. In other
> words, min_seq[type] == lrugen->min_seq[type]. Then we should return
> directly to avoid unnecessary overhead later.
>
> Corollary: If min_seq[type] of both anonymous and file is not increased,
> try_to_inc_min_seq() will fail.
>
> Proof:
> It is known that min_seq[type] has not increased, that is, min_seq[type]
> is equal to lrugen->min_seq[type], then the following:
>
> case 1: min_seq[type] has not been reassigned and changed before
> judgment min_seq[type] <= lrugen->min_seq[type].
> Then the subsequent min_seq[type] <= lrugen->min_seq[type] judgment
> will always be true.
>
> case 2: min_seq[type] is reassigned to seq, before judgment
> min_seq[type] <= lrugen->min_seq[type].
> Then at least the condition of min_seq[type] > seq must be met
> before min_seq[type] will be reassigned to seq.
> That is to say, before the reassignment, lrugen->min_seq[type] > seq
> is met, and then min_seq[type] = seq.
> Then the following min_seq[type](seq) <= lrugen->min_seq[type] judgment
> is always true.
>
> Therefore, in try_to_inc_min_seq(), If min_seq[type] of both anonymous
> and file is not increased, we can return false directly to avoid
> unnecessary overhead.
>
> Suggested-by: Yuanchu Xie <yuanchu@google.com>
> Signed-off-by: Hao Jia <jiahao1@lixiang.com>
> ---
> v1 to v2:
>  - Modify commit message to make proof clearer.
>  - Use one bool to detect any increments in min_seq[type], suggested by Yuanchu Xie.
>
> Link to v1: https://lore.kernel.org/all/20250630080603.36171-1-jiahao.kernel@gmail.com
>
>  mm/vmscan.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index d48e8d365502..d5b6924aeb8f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3919,6 +3919,7 @@ static bool 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);
>
> @@ -3935,11 +3936,20 @@ static bool try_to_inc_min_seq(struct lruvec *lruvec, int swappiness)
>                         }
>
>                         min_seq[type]++;
> +                       seq_inc_flag = true;
>                 }
>  next:
>                 ;
>         }
>
> +       /*
> +        * If min_seq[type] of both anonymous and file is not increased,
> +        * we can directly return false to avoid unnecessary checking
> +        * overhead later.
> +        */

I think we can make this comment (and probably the commit message) even simpler.

try_to_inc_min_seq (and inc_min_seq) are both called under lru_lock,
so I don't think there can be any concurrent modifications going on.
(Note readers don't necessarily take the lock though.) So I think we
can simply state it like:

"If we didn't change any min_seq, we can return immediately."

This patch had caused me to think we can do even more to make this
function simpler, but I spent an hour fiddling with it and I don't
have any concrete suggestions. So, I see no reason to hold up this
patch, it seems straightforwardly correct to me. Any further
simplification could just be a follow up patch later.

You can take:
Reviewed-by: Axel Rasmussen <axelrasmussen@google.com>

> +       if (!seq_inc_flag)
> +               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
>