Re: [PATCH v3] mm: mglru: fix stale batch updates after memcg reparenting

[Harry Yoo]

On 6/26/26 1:48 PM, Qi Zheng wrote:
> 
> 
> On 6/26/26 12:43 PM, Harry Yoo wrote:
>>
>>
>> On 6/26/26 11:27 AM, Qi Zheng wrote:
>>> Hi Johannes,
>>>
>>> On 6/26/26 2:41 AM, Johannes Weiner wrote:
>>>> On Thu, Jun 25, 2026 at 11:15:54PM +0800, Qi Zheng wrote:
>>>>> From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
>>>>>
>>>>> The mglru page table walker batches per-generation size deltas in
>>>>> walk->nr_pages while walking page tables without holding the lruvec
>>>>> lock.
>>>>> The reset_batch_size() later folds those deltas into walk->lruvec
>>>>> under
>>>>> the lruvec lock.
>>>>>
>>>>> The page table walker can run concurrently with the memcg reparenting
>>>>> path
>>>>> as follows:
>>>>>
>>>>> CPU0                           CPU1
>>>>> ====                           ====
>>>>>
>>>>> walk_mm
>>>>> --> walk_page_range
>>>>>       --> update_batch_size
>>>>>           --> walk->nr_pages += delta
>>>>>
>>>>>                                 mem_cgroup_css_offline
>>>>>                                 --> memcg_reparent_objcgs
>>>>>                                     --> lock lruvec
>>>>>                                         lru_gen_reparent_memcg
>>>>>                                         --> reparent child folios to
>>>>> parent
>>>>>                                         unlock lruvec
>>>>>
>>>>>       lock lruvec
>>>>>       reset_batch_size
>>>>>       --> child lrugen->nr_pages += delta
>>>>>
>>>>> This will trigger the following warning in lru_gen_exit_memcg():
>>>>>
>>>>>      VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
>>>>>                     sizeof(lruvec->lrugen.nr_pages)));
>>>>>
>>>>> And the user-visible impact of underestimated nr_pages in MGLRU was
>>>>> premature OOMs because MGLRU does not try to reclaim memory when
>>>>> nr_pages
>>>>> reaches zero, but there are still more pages.
>>>>>
>>>>> To fix it, make reset_batch_size() check CSS_DYING under RCU before
>>>>> flushing the pending batch. A non-dying memcg keeps the original
>>>>> lruvec
>>>>> stable against RCU-delayed offlining; a dying memcg redirects the
>>>>> deltas
>>>>> to the first non-dying ancestor.
>>>>>
>>>>> Reported-by: Peiyang He <peiyang_he@xxxxxxxxxxxxxxxx>
>>>>> Closes: https://lore.kernel.org/all/5A9E929D82717101+12fcf643-
>>>>> efb8-4b9a-a53a-1e28cc894f0b@xxxxxxxxxxxxxxxx
>>>>> Fixes: f304652609ea ("mm: vmscan: prepare for reparenting MGLRU
>>>>> folios")
>>>>> Cc: <stable@xxxxxxxxxxxxxxx>
>>>>> Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
>>>>> ---
>>>>> Changes in v3:
>>>>>    - re-implement lock_batch_lruvec() by checking CSS_DYING under the
>>>>> RCU lock
>>>>>      (suggested by Harry)
>>>>>    - update the commit message (suggested by Harry)
>>>>>    - temporarily drop the previous Reviewed-by tags
>>>>>      (since the sync method has changed)
>>>>>    - rebase onto the next-20260624
>>>>>
>>>>> Changes in v2:
>>>>>    - update the commit message (pointed by Barry)
>>>>>    - collect Reviewed-by
>>>>>
>>>>>    mm/vmscan.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>>>>>    1 file changed, 38 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>>> index 35c3bb15ae96..1ec8c23c72b9 100644
>>>>> --- a/mm/vmscan.c
>>>>> +++ b/mm/vmscan.c
>>>>> @@ -3262,10 +3262,44 @@ static void update_batch_size(struct
>>>>> lru_gen_mm_walk *walk, struct folio *folio,
>>>>>        walk->nr_pages[new_gen][type][zone] += delta;
>>>>>    }
>>>>>    +#ifdef CONFIG_MEMCG
>>>>> +static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)
>>>>> +{
>>>>> +    struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>>>> +    struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>>>> +
>>>>> +    rcu_read_lock();
>>>>
>>>> Where is this unlocked?
>>>
>>> The lruvec_unlock_irq() in reset_batch_size() will handle the unlocking.
>>>
>>>>
>>>>> +    /*
>>>>> +     * The memcg can be NULL when the memory controller is disabled.
>>>>> +     * Otherwise, the caller keeps the memcg owning @lruvec alive.
>>>>> +     */
>>>>> +    if (!memcg || !css_is_dying(&memcg->css))
>>>>> +        goto lock;
>>>>> +
>>>>> +    do {
>>>>> +        memcg = parent_mem_cgroup(memcg);
>>>>> +    } while (memcg && css_is_dying(&memcg->css));
>>>>> +    lruvec = mem_cgroup_lruvec(memcg, pgdat);
>>>>
>>>>      while (unlikely(memcg && css_is_dying(&memcg->css))) {
>>>>          memcg = parent_mem_cgroup(memcg);
>>>>          lruvec = mem_cgroup_lruvec(memcg, pgdat);
>>>
>>> There is no need to acquire the lruvec before finding the first
>>> non-dying memcg.
>>
>> struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>> struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>
>> rcu_read_lock()
>>
>> while (unlikely(memcg_is_dying(memcg)))
>>          memcg = parent_mem_cgroup(memcg);
>>
>> lruvec = mem_cgroup_lruvec(memcg, pgdat);
> 
> If the first memcg is already non-dying, there's no need to re-acquire
> the lruvec. ;)

Oh, right :)

Hmm but I still think Johannes' suggestion makes the code cleaner.
Observing a dying cgroup should be rare anyway, it's worth focusing
more on readability?

-- 
Cheers,
Harry / Hyeonggon

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature