On 05/22/2017 01:13 PM, Waiman Long wrote: > On 05/19/2017 04:26 PM, Tejun Heo wrote: >>> @@ -2982,22 +3010,48 @@ static int cgroup_enable_threaded(struct cgroup *cgrp) >>> LIST_HEAD(csets); >>> struct cgrp_cset_link *link; >>> struct css_set *cset, *cset_next; >>> + struct cgroup *child; >>> int ret; >>> + u16 ss_mask; >>> >>> lockdep_assert_held(&cgroup_mutex); >>> >>> /* noop if already threaded */ >>> - if (cgrp->proc_cgrp) >>> + if (cgroup_is_threaded(cgrp)) >>> return 0; >>> >>> - /* allow only if there are neither children or enabled controllers */ >>> - if (css_has_online_children(&cgrp->self) || cgrp->subtree_control) >>> + /* >>> + * Allow only if it is not the root and there are: >>> + * 1) no children, >>> + * 2) no non-threaded controllers are enabled, and >>> + * 3) no attached tasks. >>> + * >>> + * With no attached tasks, it is assumed that no css_sets will be >>> + * linked to the current cgroup. This may not be true if some dead >>> + * css_sets linger around due to task_struct leakage, for example. >>> + */ >> It doesn't look like the code is actually making this (incorrect) >> assumption. I suppose the comment is from before >> cgroup_is_populated() was added? > Yes, it is a bug. I should have checked the tasks_count instead of using > cgroup_is_populated. Thanks for catching that. Sorry, I would like to take it back. I think cgroup_is_populated() will be set if there is any task attached to the cgroup. So I think it is doing the right thing with regard to (3). Cheers, Longman -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
