Re: [PATCH v5 2/5] cgroup: use separate rstat trees for each subsystem

[JP Kobryn <inwardvessel@gmail.com>]

On 5/7/25 2:24 AM, Yosry Ahmed wrote:
> On Fri, May 02, 2025 at 05:12:19PM -0700, JP Kobryn wrote:
[..]
>> @@ -6101,6 +6087,8 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early)
>>   	} else {
>>   		css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2, GFP_KERNEL);
>>   		BUG_ON(css->id < 0);
>> +
>> +		BUG_ON(css_rstat_init(css));
> 
> We call css_rstat_init() here for subsys css's that are not early
> initialized, and in cgroup_setup_root() self css's. We can probably move
> both calls into cgroup_init() as I mentioned earlier?

I think it should stay here for two reasons. The first is because it
must precede the call to online_css(). There is a stated assumption that
css->css_online() is called (within online_css()) "after cgrp
successfully completed all allocations" in the admin-guide. There is an
example of this assumption in memcg_cgroup_css_online() in which the
work for periodic flushing is enqueued. So if online_css() is called
before css_rstat_init() there would be a race between flushing and 
initializing rstat for the css.
The second reason is that calling css_rstat_init() on a subsystem css
must also follow the call to init_and_link_css(), or else the css will
not have any subsystem association at the point of rstat init. It would
result in a subsystem css appearing to be the cgroup::self css since the
cgroup_subsys_state::ss field would be NULL.

> 
> Also, I think this version just skips calling css_rstat_init() for early
> initialized subsys css's, without adding the patch that you talked about
> earlier which protects against early initialized subsystems using rstat.
> 

Right, that was a pre-existing constraint but I'll create that patch and
prepend it to this series in the next rev.