Re: [PATCH v3 28/30] mm: memcontrol: prepare for reparenting state_local

[Qi Zheng]

On 1/29/26 10:10 AM, Harry Yoo wrote:
> On Mon, Jan 19, 2026 at 11:34:53AM +0800, Qi Zheng wrote:
> > On 1/18/26 11:20 AM, Shakeel Butt wrote:
> > > On Wed, Jan 14, 2026 at 07:32:55PM +0800, Qi Zheng wrote:
> > > > From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> > > >
> > > > To resolve the dying memcg issue, we need to reparent LRU folios of child
> > > > memcg to its parent memcg. The following counts are all non-hierarchical
> > > > and need to be reparented to prevent the counts of parent memcg overflow.
> > > >
> > > > 1. memcg->vmstats->state_local[i]
> > > > 2. pn->lruvec_stats->state_local[i]
> > > >
> > > > This commit implements the specific function, which will be used during
> > > > the reparenting process.
> > >
> > > Please add more explanation which was discussed in the email chain at
> > > https://lore.kernel.org/all/5dsb6q2r4xsi24kk5gcnckljuvgvvp6nwifwvc4wuho5hsifeg@5ukg2dq6ini5/
> >
> > OK, will do.
> >
> > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > index 70583394f421f..7aa32b97c9f17 100644
> > > > --- a/mm/memcontrol.c
> > > > +++ b/mm/memcontrol.c
> > > > @@ -225,6 +225,28 @@ static inline struct obj_cgroup *__memcg_reparent_objcgs(struct mem_cgroup *memc
> > > >    	return objcg;
> > > >    }
> > > > +#ifdef CONFIG_MEMCG_V1
> > > > +static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force);
> > > > +
> > > > +static inline void reparent_state_local(struct mem_cgroup *memcg, struct mem_cgroup *parent)
> > > > +{
> > > > +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
> > > > +		return;
> > > > +
> > > > +	synchronize_rcu();
> > >
> > > Hmm synchrinuze_rcu() is a heavy hammer here. Also you would need rcu
> > > read lock in mod_memcg_state() & mod_memcg_lruvec_state() for this
> > > synchronize_rcu().
> >
> > Since these two functions require memcg or lruvec, they are already
> > within the critical section of the RCU lock.
>
> What happens if someone grabbed a refcount and then release the rcu read
> lock before percpu refkill and then call mod_memcg[_lruvec]_state()?
>
> In this case, can we end up reparenting in the middle of non-hierarchical
> stat update because they don't have RCU grace period?
>
> Something like
>
> T1				T2
>
> 				- rcu_read_lock()
> 				- get memcg refcnt
> 				- rcu_read_unlock()
>
> 				- call mod_memcg_state()
> 				- CSS_IS_DYING is not set
> - Set CSS_IS_DYING
> - Trigger percpu refkill
> 				
> - Trigger offline_css()
>    -> reparent non-hierarchical	- update non-hierarchical stats
>       stats
> 				- put memcg refcount

Good catch, I think you are right.

The rcu lock should be added to mod_memcg_state() and
mod_memcg_lruvec_state().

I will update to v4 as soon as possible.

Thanks,
Qi

> > > Hmm instead of synchronize_rcu() here, we can use queue_rcu_work() in
> > > css_killed_ref_fn(). It would be as simple as the following:
> >
> > It does look much simpler, will do.
> >
> > Thanks,
> > Qi