Re: [PATCH v4 4/5] cgroup: use separate rstat trees for each subsystem

[JP Kobryn <inwardvessel@gmail.com>]

On 4/15/25 10:15 AM, Michal Koutný wrote:
> On Thu, Apr 03, 2025 at 06:10:49PM -0700, JP Kobryn <inwardvessel@gmail.com> wrote:
>> --- a/kernel/cgroup/cgroup.c
>> +++ b/kernel/cgroup/cgroup.c
> ...
>> @@ -5425,6 +5417,9 @@ static void css_free_rwork_fn(struct work_struct *work)
>>   		struct cgroup_subsys_state *parent = css->parent;
>>   		int id = css->id;
>>   
>> +		if (ss->css_rstat_flush)
>> +			css_rstat_exit(css);
>> +
> 
> It should be safe to call this unguarded (see also my comment below at
> css_rstat_flush()).

Hmmm I checked my initial assumptions. I'm still finding that css's from
any subsystem regardless of rstat usage can reach this call to exit.
Without the guard there will be undefined behavior.

> 
>>   		ss->css_free(css);
>>   		cgroup_idr_remove(&ss->css_idr, id);
>>   		cgroup_put(cgrp);
>> @@ -5477,11 +5472,8 @@ static void css_release_work_fn(struct work_struct *work)
>>   	if (ss) {
>>   		struct cgroup *parent_cgrp;
>>   
>> -		/* css release path */
>> -		if (!list_empty(&css->rstat_css_node)) {
>> +		if (ss->css_rstat_flush)
>>   			css_rstat_flush(css);
>> -			list_del_rcu(&css->rstat_css_node);
>> -		}
> 
> Ditto.

Same as mentioned above, there are cases where some css's belonging to a
subsystem like "cpu" (via CONFIG_CGROUP_SCHED) can reach this code.

> 
> These conditions -- css_is_cgroup(pos) and pos->ss->css_rstat_flush
> should be invariant wrt pos in the split tree, right?

It's a good point. Yes, I would say so.

> It's a μoptimization but may be worth checking only once before
> processing the update tree?

Let me do that that in the next rev.