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()?
>
> > +
> > + 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,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.
>
> > +
> > +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;
> > }
>
