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

[Qi Zheng]

On 1/29/26 8:23 PM, Harry Yoo wrote:
> On Thu, Jan 29, 2026 at 04:50:39PM +0800, Qi Zheng wrote:
> > 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().
>
> Thanks for confirming!
>
> Because it's quite confusing, let me ask few more questions...
>
> Q1. Yosry mentioned [1] [2] that stat updates should be done in the same
> RCU section that is used to grab a refcount of the cgroup.
>
> But I don't think your work is relying on that. Instead, I guess, it's
> relying on the CSS_DYING check from reader side to determine whether it

Only relying the CSS_DYING check is insufficient. Otherwise, the
following race may occur:

T1				T2

				memcg_is_dying is false
Set CSS_IS_DYING
reparent non-hierarchical	update non-hierarchical stats for child

So IIUC we should add rcu lock, then:

T1				T2

				rcu_read_lock
				memcg_is_dying is false

Set CSS_IS_DYING
				update non-hierarchical stats for child
				rcu_read_unlock

synchronize_rcu or rcu work
--> reparent non-hierarchical


> should update stats of the child or parent memcg, right?
>
> -> That being said, when rcu_read_lock() is called _after_ stats are
>     reparented, the reader must observe that the CSS_DYING flag is set.
>
> [1] https://lore.kernel.org/all/utl6esq7jz5e4f7kwgrpwdjc2rm3yi33ljb6xkm5nxzufa4o7s@hblq2piu3vnz
> [2] https://lore.kernel.org/all/ebdhvcwygvnfejai5azhg3sjudsjorwmlcvmzadpkhexoeq3tb@5gj5y2exdhpn
>
> Q2. When a reader checks CSS_DYING flag, how is the flag change
> guaranteed to be visible to the reader without any lock, memory barrier,
> or atomic ops involved?

The main situation requiring CSS_DYING check is as follow:

T1				T2

Set CSS_IS_DYING

synchronize_rcu or rcu work
--> reparent non-hierarchical
				rcu_read_lock()
				memcg_is_dying is true
				update non-hierarchical stats for parent

Referring to the "Memory-Barrier Guarantees" section in [1], 
synchronize_rcu() can guarantee that T2 can see CSS_IS_DYING. Right?


[1]. 
https://www.kernel.org/doc/Documentation/RCU/Design/Requirements/Requirements.rst

Thanks,
Qi

> As Shakeel mentioned elsewhere, I hope some explanations for correctness
> to be included in the commit message :)
>
> > I will update to v4 as soon as possible.
>
> Thanks a lot!
>
> I'll wait for that and will review carefully to make sure it's correct ;)
>
> > 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