Re: [PATCH v4 4/5] cgroup: use separate rstat trees for each subsystem

[JP Kobryn <inwardvessel@gmail.com>]

On 4/22/25 6:33 AM, Yosry Ahmed wrote:
> On Thu, Apr 03, 2025 at 06:10:49PM -0700, JP Kobryn wrote:
[..]
>> @@ -5425,6 +5417,9 @@ static void css_free_rwork_fn(struct work_struct *work)
>>   		struct cgroup_subsys_state *parent = css->parent;
>>   		int id = css->id;
>>   
>> +		if (ss->css_rstat_flush)
>> +			css_rstat_exit(css);
>> +
> 
> This call now exists in both branches (self css or not), so it's
> probably best to pull it outside. We should probably also pull the call
> in cgroup_destroy_root() outside into css_free_rwork_fn() so that we end
> up with a single call to css_rstat_exit() (apart from failure paths).

This can be done if css_rstat_exit() is modified to guard against
invalid css's like being a subsystem css and not having implemented
css_rstat_flush.

> 
> We can probably also use css_is_cgroup() here instead of 'if (ss)' for
> consistency.
> 
>>   		ss->css_free(css);
>>   		cgroup_idr_remove(&ss->css_idr, id);
>>   		cgroup_put(cgrp);
> [..]
>> @@ -5659,6 +5647,12 @@ static struct cgroup_subsys_state *css_create(struct cgroup *cgrp,
>>   		goto err_free_css;
>>   	css->id = err;
>>   
>> +	if (ss->css_rstat_flush) {
>> +		err = css_rstat_init(css);
>> +		if (err)
>> +			goto err_free_css;
>> +	}
>> +
>>   	/* @css is ready to be brought online now, make it visible */
>>   	list_add_tail_rcu(&css->sibling, &parent_css->children);
>>   	cgroup_idr_replace(&ss->css_idr, css, css->id);
>> @@ -5672,7 +5666,6 @@ static struct cgroup_subsys_state *css_create(struct cgroup *cgrp,
>>   err_list_del:
>>   	list_del_rcu(&css->sibling);
>>   err_free_css:
>> -	list_del_rcu(&css->rstat_css_node);
>>   	INIT_RCU_WORK(&css->destroy_rwork, css_free_rwork_fn);
>>   	queue_rcu_work(cgroup_destroy_wq, &css->destroy_rwork);
>>   	return ERR_PTR(err);
>> @@ -6104,11 +6097,17 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early)
>>   	css->flags |= CSS_NO_REF;
>>   
>>   	if (early) {
>> -		/* allocation can't be done safely during early init */
>> +		/*
>> +		 * Allocation can't be done safely during early init.
>> +		 * Defer IDR and rstat allocations until cgroup_init().
>> +		 */
>>   		css->id = 1;
>>   	} else {
>>   		css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2, GFP_KERNEL);
>>   		BUG_ON(css->id < 0);
>> +
>> +		if (ss->css_rstat_flush)
>> +			BUG_ON(css_rstat_init(css));
>>   	}
>>   
>>   	/* Update the init_css_set to contain a subsys
>> @@ -6207,9 +6206,17 @@ int __init cgroup_init(void)
>>   			struct cgroup_subsys_state *css =
>>   				init_css_set.subsys[ss->id];
>>   
>> +			/*
>> +			 * It is now safe to perform allocations.
>> +			 * Finish setting up subsystems that previously
>> +			 * deferred IDR and rstat allocations.
>> +			 */
>>   			css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2,
>>   						   GFP_KERNEL);
>>   			BUG_ON(css->id < 0);
>> +
>> +			if (ss->css_rstat_flush)
>> +				BUG_ON(css_rstat_init(css));
> 
> The calls to css_rstat_init() are really difficult to track. Let's
> recap, before this change we had two calls:
> - In cgroup_setup_root(), for root cgroups.
> - In cgroup_create(), for non-root cgroups.
> 
> This patch adds 3 more, so we end up with 5 calls as follows:
> - In cgroup_setup_root(), for root self css's.
> - In cgroup_create(), for non-root self css's.
> - In cgroup_subsys_init(), for root subsys css's without early
>    initialization.
> - In cgroup_init(), for root subsys css's with early
>    initialization, as we cannot call it from cgroup_subsys_init() early
>    as allocations are not allowed during early init.
> - In css_create(), for non-root non-self css's.
> 
> We should try to consolidate as much as possible. For example:
> - Can we always make the call for root subsys css's in cgroup_init(),
>    regardless of early initialization status? Is there a need to make the
>    call early for subsystems that use early in cgroup_subsys_init()
>    initialization?
> 
> - Can we always make the call for root css's in cgroup_init(),
>    regardless of whether the css is a self css or a subsys css? I imagine
>    we'd still need two separate calls, one outside the loop for the self
>    css's, and one in the loop for subsys css's, but having them in the
>    same place should make things easier.

The answer might be the same for the two questions above. I think at
best, we can eliminate the single call below to css_rstat_init():

cgroup_init()
	for_each_subsys(ss, ssid)
		if (ss->early_init)
			css_rstat_init(css) /* remove */

I'm not sure if it's by design and also an undocumented constraint but I
find that it is not possible to have a cgroup subsystem that is
designated for "early init" participate in rstat at the same time. The
necessary ordering of actions should be:

init_and_link_css(css, ss, ...)
css_rstat_init(css)
css_online(css)

This sequence occurs within cgroup_init_subsys() when ss->early_init is
false. However, when early_init is true, the last two calls are
effectively inverted:

css_online(css)
css_rstat_init(css) /* too late */

This needs to be avoided or else failures will occur during boot.

Note that even before this series, this constraint seems to have
existed. Using branch for-6.16 as a base, I experimented with a minimal
custom test cgroup in which I implement css_rstat_flush while early_init
is on. The system fails during boot because online_css() is called
before cgroup_rstat_init().

cgroup_init_early()
	for_each_subsys(ss, ssid)
		if (ss->early)
			cgroup_init_subsys(ss, true)
				css = ss->css_alloc()
				online_css(css)
cgroup_init()
	cgroup_setup_root()
		cgroup_rstat_init(root_cgrp) /* too late */

Unless I"m missing something, do you think this constraint is worthy of
a separate patch? Something like this that prevents the combination of
rstat with early_init:

--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6130,7 +6130,8 @@ int __init cgroup_init_early(void)
     for_each_subsys(ss, i) {
-       WARN(!ss->css_alloc || !ss->css_free || ss->name || ss->id,
+       WARN(!ss->css_alloc || !ss->css_free || ss->name || ss->id ||
+           (ss->early_init && ss->css_rstat_flush),

> 
> Ideally if we can do both the above, we'd end up with 3 calling
> functions only:
> - cgroup_init() -> for all root css's.
> - cgroup_create() -> for non-root self css's.
> - css_create() -> for non-root subsys css's.
> 
> Also, we should probably document all the different call paths for
> css_rstat_init() and css_rstat_exit() somewhere.

Will do.