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

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

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@bytedance.com>
>>>>>>
>>>>>> 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
>