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

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

[Hao Jia]

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.
+	 */
+	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

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

[Axel Rasmussen]

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
>

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

[Hao Jia]

On 2025/7/11 06:49, Axel Rasmussen wrote:
> 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>
> 

Hi, Andrew

Could you please replace the V1 version in mm-unstable branch with this 
patch v2?

https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/commit/?h=mm-unstable&id=0163463f9dcc965060c5f51de69727d1b21d2b2b

Patch v2 looks more reasonable.

Thanks,
Hao



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