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

[Hao Jia <jiahao.kernel@gmail.com>]

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