On Tue, Oct 17, 2023 at 12:45:38PM +0000, Yafang Shao <laoar.shao@xxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
