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

[Yosry Ahmed <yosry.ahmed@linux.dev>]

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