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

JP Kobryn
2025-02-25T19:20:52Z

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;
>>> }
>>