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

[Johannes Weiner]

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@xxxxxxxxx> 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>
> >> Reviewed-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>
> >> ---
> >> 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()?