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