Re: [PATCH 03/11] cgroup: move cgroup_rstat from cgroup to cgroup_subsys_state

[JP Kobryn <inwardvessel@gmail.com>]

On 2/20/25 9:22 AM, Yosry Ahmed wrote:
> On Thu, Feb 20, 2025 at 09:06:44AM -0800, Shakeel Butt wrote:
>> On Mon, Feb 17, 2025 at 07:14:40PM -0800, JP Kobryn wrote:
>> [...]
>>> @@ -3240,6 +3234,12 @@ static int cgroup_apply_control_enable(struct cgroup *cgrp)
>>>   				css = css_create(dsct, ss);
>>>   				if (IS_ERR(css))
>>>   					return PTR_ERR(css);
>>
>> Since rstat is part of css, why not cgroup_rstat_init() inside
>> css_create()?

Good idea. Will do that in v2.
>>
>>> +
>>> +				if (css->ss && css->ss->css_rstat_flush) {
>>> +					ret = cgroup_rstat_init(css);
>>> +					if (ret)
>>> +						goto err_out;
>>> +				}
>>>   			}
>>>   
>>>   			WARN_ON_ONCE(percpu_ref_is_dying(&css->refcnt));
>>> @@ -3253,6 +3253 css when it is destroyed, so moving the cleanup to css_kill()
should handle th,21 @@ static int cgroup_apply_control_enable(struct 
cgroup *cgrp)
>>>   	}
>>>   
>>>   	return 0;
>>
>> Why not the following cleanup in css_kill()? If you handle it in
>> css_kill(), you don't need this special handling.
> 
> Also, I don't think we are currently calling cgroup_rstat_exit() for
> every css when it is destroyed, so moving the cleanup to css_kill()
> should handle this as well.

Thanks, this makes sense and will eliminate the need for the extra 
cleanup here. It seems that instead of kill_css(), the 
css_free_rwork_fn() function is a good place to call css_rstat_exit(), 
since it corresponds with calling cgroup_rstat_exit() when the css is
the "self" base stats.
> 
>>
>>> +
>>> +err_out:
>>> +	cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) {
>>> +		for_each_subsys(ss, ssid) {
>>> +			struct cgroup_subsys_state *css = cgroup_css(dsct, ss);
>>> +
>>> +			if (!(cgroup_ss_mask(dsct) & (1 << ss->id)))
>>> +				continue;
>>> +
>>> +			if (css && css->ss && css->ss->css_rstat_flush)
>>> +				cgroup_rstat_exit(css);
>>> +		}
>>> +	}
>>> +
>>> +	return ret;
>>>   }
>>