Re: [PATCH 2/4 v3] cgroup: use separate rstat trees for each subsystem

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Mar 19, 2025 at 03:21:48PM -0700, JP Kobryn wrote:
> Different subsystems may call cgroup_rstat_updated() within the same
> cgroup, resulting in a tree of pending updates from multiple subsystems.
> When one of these subsystems is flushed via cgroup_rstat_flushed(), all
> other subsystems with pending updates on the tree will also be flushed.
> 
> Change the paradigm of having a single rstat tree for all subsystems to
> having separate trees for each subsystem. This separation allows for
> subsystems to perform flushes without the side effects of other subsystems.
> As an example, flushing the cpu stats will no longer cause the memory stats
> to be flushed and vice versa.
> 
> In order to achieve subsystem-specific trees, change the tree node type
> from cgroup to cgroup_subsys_state pointer. Then remove those pointers from
> the cgroup and instead place them on the css. Finally, change the
> updated/flush API's to accept a reference to a css instead of a cgroup.
> This allows a specific subsystem to be associated with an update or flush.
> Separate rstat trees will now exist for each unique subsystem.
> 
> Since updating/flushing will now be done at the subsystem level, there is
> no longer a need to keep track of updated css nodes at the cgroup level.
> The list management of these nodes done within the cgroup (rstat_css_list
> and related) has been removed accordingly. There was also padding in the
> cgroup to keep rstat_css_list on a cacheline different from
> rstat_flush_next and the base stats. This padding has also been removed.

Overall, this looks okay but I think the patch should be split further.
There's too much cgroup -> css renames mixed with actual changes which makes
it difficult to understand what the actual changes are. Can you please
separate it into a patch which makes everything css based but the actual
queueing and flushing is still only on the cgroup css and then the next
patch to actually split out linking and flushing to each css?

> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 13fd82a4336d..4e71ae9858d3 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -346,6 +346,11 @@ static inline bool css_is_dying(struct cgroup_subsys_state *css)
>  	return !(css->flags & CSS_NO_REF) && percpu_ref_is_dying(&css->refcnt);
>  }
>  
> +static inline bool css_is_cgroup(struct cgroup_subsys_state *css)
> +{
> +	return css->ss == NULL;
> +}

Maybe introduce this in a prep patch and replace existing users?

...
> @@ -6082,11 +6077,16 @@ 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().
> +		 */

Nit: Please use fully winged comment blocks for multilines with
captalizations.

Thanks.

-- 
tejun