On Mon, Aug 15, 2011 at 08:49:57PM +0200, Oleg Nesterov wrote: > On 07/29, Ben Blum wrote: > > > > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is > > not sufficient to guarantee the tasklist is stable w.r.t. de_thread and > > exit. Taking tasklist_lock for reading, instead of rcu_read_lock, > > ensures proper exclusion. > > Yes. > > So far I still think we should fix while_each_thread() so that it works > under rcu_read_lock() "as exepected", I'll try to think more. > > But whatever we do with while_each_thread(), this can't help > cgroup_attach_proc(), it needs the locking. > > > - rcu_read_lock(); > > + read_lock(&tasklist_lock); > > if (!thread_group_leader(leader)) { > > Agreed, this should work. > > But can't we avoid the global list? thread_group_leader() or not, we do > not really care. We only need to ensure we can safely find all threads. > > How about the patch below? > > > With or without this/your patch this leader can die right after we > drop the lock. ss->can_attach(leader) and ss->attach(leader) look > suspicious. If a sub-thread execs, this task_struct has nothing to > do with the threadgroup. > > > > Also. This is off-topic, but... Why cgroup_attach_proc() and > cgroup_attach_task() do ->attach_task() + cgroup_task_migrate() > in the different order? cgroup_attach_proc() looks wrong even > if currently doesn't matter. Right. As we concluded in our off-list discussion, if there is no strong reason for that, I'm going to fix that in my task counter patchset because there it really matters. If we can't migrate the thread because it has already exited, we really don't want to call ->attach_task() but rather cancel_attach_task(). Thanks. _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers
