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

[Yosry Ahmed <yosry.ahmed@linux.dev>]

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.

> @@ -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").

Otherwise this LGTM.