Re: [RFC PATCH 2/3] mm/mglru: skip the flags cmpxchg when nothing changes

[Ridong Chen <ridong.chen@linux.dev>]

On 11/7/2026 6:39 PM, Kairui Song wrote:
> On Fri, Nov 6, 2026 at 9:44 AM Ridong Chen <ridong.chen@linux.dev> wrote:
>>
>> On 11/6/2026 12:21 AM, Kairui Song wrote:
>>> On Thu, Nov 5, 2026 at 10:11 PM Ridong Chen <ridong.chen@linux.dev> wrote:
>>>>
>>>> From: Ridong Chen <chenridong@xiaomi.com>
>>>>
>>>> folio_update_gen() and folio_set_lru_refs() always end with a
>>>> try_cmpxchg() on folio->flags, even when the new value equals the old
>>>> one. The locked instruction still takes the struct page cacheline in
>>>> exclusive state.
>>>>
>>>> That is common in practice: a folio mapped by many processes is visited
>>>> once per mm during a page table walk, and after the first visit it is
>>>> already in the youngest generation with refs cleared and PG_workingset
>>>> set. isolate_folio() and the eviction paths also call
>>>> folio_set_lru_refs(folio, 0) on folios whose refs are already zero.
>>>>
>>>> Break out of the loop when the flags would not change. The result is
>>>> the same as a successful no-op cmpxchg. The only difference is that the
>>>> full barrier implied by the locked instruction is gone on that path.
>>>> No caller relies on it: the eviction and isolation callers run under
>>>> the LRU lock, and the others (page table walks, folio_mark_accessed(),
>>>> lru_gen_clear_refs()) only care about the resulting flags value, which
>>>> is already in place. Note that lru_gen_inc_refs() already returns
>>>> without any atomic when refs is at LRU_REFS_MAX and the folio is in
>>>> the workingset.
>>>>
>>>> Tested on an i7-13700HX with turbo disabled and the performance
>>>> governor. N processes map the same 2GiB tmpfs file (huge=never) and
>>>> read every page, then one aging pass is triggered on their memcg
>>>> through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1").
>>>> Time per aging pass in ms, median of 5 runs of 10 passes each, averaged
>>>> over 2 boots of each kernel:
>>>>
>>>>     processes    base   patched
>>>>             1    32.9      31.7   -3.5%
>>>>             8   195.1     160.2  -17.9%
>>>>            32   755.8     592.2  -21.7%
>>>>            64  1597.2    1260.6  -21.1%
>>>>
>>>> With one process, every visit changes the generation, so there is
>>>> nothing to skip. Before the patch, perf annotate attributes 53-58% of
>>>> the walk_update_folio() samples to the lock cmpxchg in the inlined
>>>> folio_update_gen() in this test.
>>>>
>>>> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
>>>> ---
>>>>    include/linux/mm_inline.h | 2 ++
>>>>    mm/vmscan.c               | 2 ++
>>>>    2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
>>>> index b3d85fe91385..4ba9079f4dc8 100644
>>>> --- a/include/linux/mm_inline.h
>>>> +++ b/include/linux/mm_inline.h
>>>> @@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
>>>>           do {
>>>>                   new_flags = old_flags;
>>>>                   lru_set_refs_flags(&new_flags, refs);
>>>> +               if (new_flags == old_flags)
>>>> +                       break;
>>>>           } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
>>>>    }
>>>>
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index 50c352e9ffef..3fa76715382d 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
>>>>                   lru_set_gen_flags(&new_flags, new_gen);
>>>>                   lru_set_refs_flags(&new_flags, 0);
>>>>                   new_flags |= BIT(PG_workingset);
>>>> +               if (new_flags == old_flags)
>>>> +                       break;
>>>>           } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
>>>
>>> Hi Ridong,
>>>
>>> I had the same idea in V4 of MGLRU-FG which is posted a few weeks ago,
>>> with the same code:
>>> https://lore.kernel.org/linux-mm/20261016-mglru-fg-v4-4-2106750c0bfa@tencent.com/
>>>
>>> + if (new_flags == old_flags)
>>> + break;
>>>
>>
>> Hi Kairui,
>>
>> Oh. Sorry about that. I read the series a few weeks ago, but as it is large
>> I missed this change. I should have checked it again before sending.
>>
>>> I didn't have time to benchmark that one function but the e2e test
>>> showed very trivial improvement, it's a good improvement though. Feel
>>> free to add:
>>>
>>> Reviewed-by: Kairui Song <kasong@tencent.com>
>>>
>>
>> Thanks for the Reviewed-by. If you don't mind, I'd suggest merging it as
>> a separate patch, as the test shows the benefit of this small change,
>> which makes the code easier to trace. I will also mention in the commit
>> message that it is a small part of your series. Does that sound OK to
>> you?
> 
> Sounds good to me! No need to mention that, though; I believe you came
> up with this idea by yourself. And that will make the change more
> traceable indeed.

That's very kind of you. Thanks.

-- 
Best regards
Ridong