Re: [PATCH v4 2/5] cgroup: add helper for checking when css is cgroup::self

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

On Thu, Apr 03, 2025 at 06:10:47PM -0700, JP Kobryn wrote:
> The cgroup struct has a css field called "self". The main difference
> between this css and the others found in the cgroup::subsys array is that
> cgroup::self has a NULL subsystem pointer. There are several places where
> checks are performed to determine whether the css in question is
> cgroup::self or not. Instead of accessing css->ss directly, introduce a
> helper function that shows the intent and use where applicable.
> 
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> ---
>  include/linux/cgroup.h | 5 +++++
>  kernel/cgroup/cgroup.c | 4 ++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 28e999f2c642..7c120efd5e49 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -347,6 +347,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)

I think css_is_self() or css_is_cgroup_self() may be clearer given that
we are basically checking if css is the same as css->cgroup->self. As I
write this out, I am wondering why don't we check css ==
css->cgroup->self instead (and perhaps add a WARN to make sure css->ss
is NULL as expected)?

This seems clearer to me unless I am missing something.

> +{
> +	return css->ss == NULL;
> +}
> +
>  static inline void cgroup_get(struct cgroup *cgrp)
>  {
>  	css_get(&cgrp->self);
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 77349d07b117..00eb882dc6e7 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -1719,7 +1719,7 @@ static void css_clear_dir(struct cgroup_subsys_state *css)
>  
>  	css->flags &= ~CSS_VISIBLE;
>  
> -	if (!css->ss) {
> +	if (css_is_cgroup(css)) {
>  		if (cgroup_on_dfl(cgrp)) {
>  			cgroup_addrm_files(css, cgrp,
>  					   cgroup_base_files, false);
> @@ -1751,7 +1751,7 @@ static int css_populate_dir(struct cgroup_subsys_state *css)
>  	if (css->flags & CSS_VISIBLE)
>  		return 0;
>  
> -	if (!css->ss) {
> +	if (css_is_cgroup(css)) {
>  		if (cgroup_on_dfl(cgrp)) {
>  			ret = cgroup_addrm_files(css, cgrp,
>  						 cgroup_base_files, true);
> -- 
> 2.47.1
> 
>