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

[JP Kobryn <inwardvessel@gmail.com>]

On 3/20/25 2:31 PM, Tejun Heo wrote:
> 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?

Sure, no problem. I did something similar in the RFC series so I'll 
apply again here.

>> 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?
Makes sense, will do.
> ...
>> @@ -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.
>