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:
[..]
>> @@ -383,32 +395,45 @@ __bpf_kfunc void css_rstat_flush(struct cgroup_subsys_state *css)
>>   
>>   int css_rstat_init(struct cgroup_subsys_state *css)
>>   {
>> -	struct cgroup *cgrp = css->cgroup;
>> +	struct cgroup *cgrp;
>>   	int cpu;
>> +	bool is_cgroup = css_is_cgroup(css);
>>   
>> -	/* the root cgrp has rstat_cpu preallocated */
>> -	if (!cgrp->rstat_cpu) {
>> -		cgrp->rstat_cpu = alloc_percpu(struct cgroup_rstat_cpu);
>> -		if (!cgrp->rstat_cpu)
>> -			return -ENOMEM;
>> -	}
>> +	if (is_cgroup) {
>> +		cgrp = css->cgroup;
> 
> You can keep 'cgrp' initialized at the top of the function to avoid the
> extra level of indentation here, right?
> 

I can move this initialization to the top but the indentation of this
conditional branch will remain. See more thoughts below related to this
block.

>>   
>> -	if (!cgrp->rstat_base_cpu) {
>> -		cgrp->rstat_base_cpu = alloc_percpu(struct cgroup_rstat_base_cpu);
>> +		/* the root cgrp has rstat_base_cpu preallocated */
>>   		if (!cgrp->rstat_base_cpu) {
>> -			free_percpu(cgrp->rstat_cpu);
>> +			cgrp->rstat_base_cpu = alloc_percpu(struct cgroup_rstat_base_cpu);
>> +			if (!cgrp->rstat_base_cpu)
>> +				return -ENOMEM;
>> +		}
>> +	} else if (css->ss->css_rstat_flush == NULL)
>> +		return 0;
> 
> We can probably just do this at the beginning of the function to be able
> to use the helper:
> 
> 	if (!css_is_cgroup(css) && css->ss->css_rstat_flush == NULL)
> 		return 0;

If we did this we would be wasting extra memory allocating the base
stats for every subsystem css. The intention behind the way I wrote it
was to optimize the memory overhead needed for split rstat trees.