On Tue, Oct 17, 2023 at 12:45:38PM +0000, Yafang Shao <laoar.shao@gmail.com> wrote:
> Therefore, making it RCU-safe would be beneficial.

Notice that whole cgroup_destroy_root() is actually an RCU callback (via
css_free_rwork_fn()). So the list traversal under RCU should alreay be
OK wrt its stability. Do you see a loophole in this argument?


>  /* iterate across the hierarchies */
>  #define for_each_root(root)						\
> -	list_for_each_entry((root), &cgroup_roots, root_list)
> +	list_for_each_entry_rcu((root), &cgroup_roots, root_list,	\
> +				!lockdep_is_held(&cgroup_mutex))

The extra condition should be constant false (regardless of
cgroup_mutex). IOW, RCU read lock is always required.

> @@ -1386,13 +1386,15 @@ static inline struct cgroup *__cset_cgroup_from_root(struct css_set *cset,
>  		}
>  	}
>  
> -	BUG_ON(!res_cgroup);
> +	WARN_ON_ONCE(!res_cgroup && lockdep_is_held(&cgroup_mutex));
>  	return res_cgroup;

Hm, this would benefit from a comment why !res_cgroup is conditionally
acceptable.

>  }
>  
>  /*
>   * look up cgroup associated with current task's cgroup namespace on the
> - * specified hierarchy
> + * specified hierarchy. Umount synchronization is ensured via VFS layer,
> + * so we don't have to hold cgroup_mutex to prevent the root from being
> + * destroyed.

I tried the similar via explicit lockdep invocation (in a thread I
linked to you previously) and VFS folks weren't ethusiastic about it.

You cannot hide this synchronization assumption in a mere comment.

Thanks,
Michal