Re: [PATCH v6 30/33] mm: memcontrol: prepare for reparenting non-hierarchical stats

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

Hi Michal,

On 3/14/26 12:22 AM, Michal Koutný wrote:
> Hello Qi.
> 
> On Thu, Mar 05, 2026 at 07:52:48PM +0800, Qi Zheng <qi.zheng@linux.dev> wrote:
>> To ensure that these non-hierarchical stats work properly, we need to
>> reparent these non-hierarchical stats after reparenting LRU folios. To
>> this end, this commit makes the following preparations:
>>
>> 1. implement reparent_state_local() to reparent non-hierarchical stats
>> 2. make css_killed_work_fn() to be called in rcu work, and implement
>>     get_non_dying_memcg_start() and get_non_dying_memcg_end() to avoid race
>>     between mod_memcg_state()/mod_memcg_lruvec_state()
>>     and reparent_state_local()
> 
> 
> css_free_rwork_fn has() already RCU deferal but we discussed some
> reasons why stats reparenting cannot be done from there. IIUC something
> like:
> 
> | reparent_state_local() must be already at css_killed_work_fn() because
> | waiting until css_free_rwork_fn() would mean the non-hierarchical
> | stats of the surrogate ancestor are outdated, e.g. underflown.
> | And the waiting until css_free_rwork_fn() may still be indefinite due
> | to non-folio references to the offlined memcg.

We need to ensure that when reparent_state_local() is called, no writer
modifies the non-hierarchical stats of child memcg, otherwise these
stats modifications may be missed.

In the v3, we used synchronize_rcu() in reparent_state_local() to ensure
this, but this could cause blocking, so in this version we changed to
this asynchronous approach.

> 
> Could this be captured in the commit (if it's correct)?
> 
> 
>> --- a/kernel/cgroup/cgroup.c
>> +++ b/kernel/cgroup/cgroup.c
>> @@ -6044,8 +6044,9 @@ int cgroup_mkdir(struct kernfs_node *parent_kn, const char *name, umode_t mode)
>>    */
>>   static void css_killed_work_fn(struct work_struct *work)
>>   {
>> -	struct cgroup_subsys_state *css =
>> -		container_of(work, struct cgroup_subsys_state, destroy_work);
>> +	struct cgroup_subsys_state *css;
>> +
>> +	css = container_of(to_rcu_work(work), struct cgroup_subsys_state, destroy_rwork);
>>   
>>   	cgroup_lock();
>>   
>> @@ -6066,8 +6067,8 @@ static void css_killed_ref_fn(struct percpu_ref *ref)
>>   		container_of(ref, struct cgroup_subsys_state, refcnt);
>>   
>>   	if (atomic_dec_and_test(&css->online_cnt)) {
>> -		INIT_WORK(&css->destroy_work, css_killed_work_fn);
>> -		queue_work(cgroup_offline_wq, &css->destroy_work);
>> +		INIT_RCU_WORK(&css->destroy_rwork, css_killed_work_fn);
>> +		queue_rcu_work(cgroup_offline_wq, &css->destroy_rwork);
>>   	}
>>   }
>>
> 
> Could this be
> 
> #ifdef CONFIG_MEMCG_V1
> 		/* See get_non_dying_memcg_start, get_non_dying_memcg_end
> 		INIT_RCU_WORK(&css->destroy_rwork, css_killed_work_fn);
> 		queue_rcu_work(cgroup_offline_wq, &css->destroy_rwork);
> #else
> 		INIT_WORK(&css->destroy_work, css_killed_work_fn);
> 		queue_work(cgroup_offline_wq, &css->destroy_work);
> #endif
> 
> ?
> 
> IOW there's no need to make the cgroup release path even more
> asynchronous without CONFIG_MEMCG_V1 (all this seems CONFIG_MEMCG_V1
> specific).

Right. But I'm wondering if it's necessary to use ifdefing, since
asynchronous methods shouldn't cause any significant drawbacks?

> 
> (+not so beautiful css_killed_work_fn ifdefing but given there are the
> variants of _start, _end)
> 
>> +#ifdef CONFIG_MEMCG_V1
>> +/*
>> + * Used in mod_memcg_state() and mod_memcg_lruvec_state() to avoid race with
>> + * reparenting of non-hierarchical state_locals.
>> + */
>> +static inline struct mem_cgroup *get_non_dying_memcg_start(struct mem_cgroup *memcg)
> 
> Nit: I think the idiomatic names are begin + end (in the meaning of
> paired parenthesis, don't look at css_task_iter_start ;-).

Both are fine for me, but changing the name requires changing
[PACTH v6 30/33] and [PACTH v6 32/33], if we need to update to v7,
I can include them.

Thanks,
Qi