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

[Qi Zheng <qi.zheng@linux.dev>]

On 7/1/26 11:59 PM, Johannes Weiner wrote:
> On Thu, Jul 02, 2026 at 12:36:42AM +0900, Harry Yoo wrote:
>>
>>
>> On 7/1/26 11:57 PM, Usama Arif wrote:
>>> On Wed,  1 Jul 2026 15:52:51 +0800 Qi Zheng <qi.zheng@linux.dev> wrote:
>>>
>>>> From: Qi Zheng <zhengqi.arch@bytedance.com>
>>>>
>>>> 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@smail.nju.edu.cn>
>>>> Closes: https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-a53a-1e28cc894f0b@smail.nju.edu.cn
>>>> Fixes: f304652609ea ("mm: vmscan: prepare for reparenting MGLRU folios")
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>>>> Reviewed-by: Harry Yoo (Oracle) <harry@kernel.org>
>>>> ---
>>>> Changes in v4:
>>>>   - re-implement lock_batch_lruvec() in a simpler way
>>>>     (suggested by Johannes and Harry)
>>>>   - collect Reviewed-by
>>>>   - rebase onto the next-20260630
>>>>
>>>> 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 | 41 ++++++++++++++++++++++++++++++++++-------
>>>>   1 file changed, 34 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index 35c3bb15ae96..ca1e2a870d51 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -3262,10 +3262,40 @@ 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();
>>>> +
>>>> +	/*
>>>> +	 * The memcg can be NULL when the memory controller is disabled.
>>>> +	 * Otherwise, the caller keeps the memcg owning @lruvec alive.
>>>> +	 */
>>>> +	while (unlikely(memcg && css_is_dying(&memcg->css))) {
>>>> +		memcg = parent_mem_cgroup(memcg);
>>>> +		lruvec = mem_cgroup_lruvec(memcg, pgdat);
>>>> +	}
>>>> +
>>>> +	spin_lock_irq(&lruvec->lru_lock);
>>>
>>> Do we need an rcu_read_unlock() here?
>>
>> lruvec_unlock_irq() does that.
> 
> Yeah, that tripped me up too. And it makes me think Shakeel was right
> after all: this should live next to the other lruvec_lock() primitives.
> 
> Sure, MGLRU is the only user, but it's still much easier to understand
> this if the code sits next to the rest of the API (and the unlock!).
> 
> lruvec_live_lock_irq()?

But lruvec_lock_irq() grabs the rcu lock too. :(