Re: [PATCH v4 3/5] cgroup: change rstat function signatures from cgroup-based to css-based

[JP Kobryn <inwardvessel@gmail.com>]

On 4/22/25 5:35 AM, Yosry Ahmed wrote:
> On Thu, Apr 03, 2025 at 06:10:48PM -0700, JP Kobryn wrote:
>> This non-functional change serves as preparation for moving to
>> subsystem-based rstat trees. To simplify future commits, change the
>> signatures of existing cgroup-based rstat functions to become css-based and
>> rename them to reflect that.
>>
>> Though the signatures have changed, the implementations have not. Within
>> these functions use the css->cgroup pointer to obtain the associated cgroup
>> and allow code to function the same just as it did before this patch. At
>> applicable call sites, pass the subsystem-specific css pointer as an
>> argument or pass a pointer to cgroup::self if not in subsystem context.
>>
>> Note that cgroup_rstat_updated_list() and cgroup_rstat_push_children()
>> are not altered yet since there would be a larger amount of css to
>> cgroup conversions which may overcomplicate the code at this
>> intermediate phase.
>>
>> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> [..]
>> @@ -5720,6 +5716,14 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name,
>>   	cgrp->root = root;
>>   	cgrp->level = level;
>>   
>> +	/*
>> +	 * Now that init_cgroup_housekeeping() has been called and cgrp->self
>> +	 * is setup, it is safe to perform rstat initialization on it.
>> +	 */
>> +	ret = css_rstat_init(&cgrp->self);
>> +	if (ret)
>> +		goto out_stat_exit;
>> +
> 
> Sorry for the late review, but this looks wrong to me. I think this
> should goto out_kernfs_remove..
> 
>>   	ret = psi_cgroup_alloc(cgrp);
>>   	if (ret)
>>   		goto out_kernfs_remove;
> 
> ..and this should goto out_stat_exit.

Thanks, will invert in separate patch.
> 
>> @@ -5790,10 +5794,10 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name,
>>   
>>   out_psi_free:
>>   	psi_cgroup_free(cgrp);
>> +out_stat_exit:
>> +	css_rstat_exit(&cgrp->self);
>>   out_kernfs_remove:
>>   	kernfs_remove(cgrp->kn);
>> -out_stat_exit:
>> -	cgroup_rstat_exit(cgrp);
>>   out_cancel_ref:
>>   	percpu_ref_exit(&cgrp->self.refcnt);
>>   out_free_cgrp:
> [..]
>> @@ -298,36 +304,41 @@ static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int cpu_in_loop)
>>   	trace_cgroup_rstat_locked(cgrp, cpu_in_loop, contended);
>>   }
>>   
>> -static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in_loop)
>> +static inline void __css_rstat_unlock(struct cgroup_subsys_state *css,
>> +		int cpu_in_loop)
>>   	__releases(&cgroup_rstat_lock)
>>   {
>> +	struct cgroup *cgrp = css->cgroup;
>> +
>>   	trace_cgroup_rstat_unlock(cgrp, cpu_in_loop, false);
>>   	spin_unlock_irq(&cgroup_rstat_lock);
>>   }
>>   
>>   /**
>> - * cgroup_rstat_flush - flush stats in @cgrp's subtree
>> - * @cgrp: target cgroup
>> + * css_rstat_flush - flush stats in @css->cgroup's subtree
>> + * @css: target cgroup subsystem state
>>    *
>> - * Collect all per-cpu stats in @cgrp's subtree into the global counters
>> + * Collect all per-cpu stats in @css->cgroup's subtree into the global counters
>>    * and propagate them upwards.  After this function returns, all cgroups in
>>    * the subtree have up-to-date ->stat.
>>    *
>> - * This also gets all cgroups in the subtree including @cgrp off the
>> + * This also gets all cgroups in the subtree including @css->cgroup off the
>>    * ->updated_children lists.
>>    *
>>    * This function may block.
>>    */
>> -__bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
>> +__bpf_kfunc void css_rstat_flush(struct cgroup_subsys_state *css)
>>   {
>> +	struct cgroup *cgrp = css->cgroup;
>>   	int cpu;
>>   
>>   	might_sleep();
>>   	for_each_possible_cpu(cpu) {
>> -		struct cgroup *pos = cgroup_rstat_updated_list(cgrp, cpu);
>> +		struct cgroup *pos;
>>   
>>   		/* Reacquire for each CPU to avoid disabling IRQs too long */
>> -		__cgroup_rstat_lock(cgrp, cpu);
>> +		__css_rstat_lock(css, cpu);
>> +		pos = cgroup_rstat_updated_list(cgrp, cpu);
> 
> Moving this call under the lock is an unrelated bug fix that was already
> done by Shakeel in commit 7d6c63c31914 ("cgroup: rstat: call
> cgroup_rstat_updated_list with cgroup_rstat_lock").

Right. I had been working off of a tree that did not yet receive the
patch. Moving forward I will be using the tj/cgroup tree.

> 
> Otherwise this LGTM.

Thanks for reviewing.