Re: + cgroups-add-functionality-to-read-write-lock-clone_thread-forking-pe r-threadgroup.patch added to -mm tree

[Oleg Nesterov <oleg@redhat.com>]

On 08/21, Paul Menage wrote:
>
> On Fri, Aug 21, 2009 at 3:45 AM, Oleg Nesterov<oleg@redhat.com> wrote:
> > In case I wasn't clear.
> >
> > Let's suppose we have subthreads T1 and T2, and we have a reference to T1.
>
> In this case, T1 is also the thread group leader.

You forced me to take a look at the next patch,
cgroups-add-ability-to-move-all-threads-in-a-process-to-a-new-cgroup-atomically.patch
which uses this helper ;) please see below.

> And we hold tasklist_lock around the entire operation. (So the
> rcu_read_lock() call is probably a red herring - Li Zefan already
> suggested that it be removed).

Yes, rcu_read_lock() is not needed under tasklist to iterate over
->thread_group.

> But you're saying that could still be a problem if tsk exits  before
> we even get to this point?
>
> My impression was that if the thread group leader exits, it hangs
> around (still attached to its thread group list) until all its threads
> have exited.

Yes, unless the non-leader execs, in this case the leader can die before
sub-thread, the execing thread becomes the new leader.

IOW. Yes, ->group_leader dies last, but exec can change ->group_leader.

> In which case as long as we're holding tasklist_lock, the
> thread group list should remain valid.

Only if it was valid before we take tasklist.



OK, cgroups-add-ability-to-move-all-threads-in-a-process-to-a-new-cgroup-atomically.patch
does

	threadgroup_sighand = threadgroup_fork_lock(leader);

	rcu_read_lock();
	list_for_each_entry_rcu(tsk, &leader->thread_group, thread_group)

and this is equally wrong afais. Hmm, and other similar
list_for_each_entry_rcu's doesn't look right. This code can do something
like

	rcu_read_lock();
	if (!tsk->sighand)	// tsk is leader or not, doesn't matter
		fail;
	list_for_each_rcu(tsk->thread_group) {}
	rcu_read_unlock();

though.



And why do we need sighand->threadgroup_fork_lock ? I gueess, to protect
against clone(CLONE_THREAD).

But. threadgroup_fork_lock() has another problem. Even if the process
P is singlethreaded, I can't see how ->threadgroup_fork_lock work.

threadgroup_fork_lock() bumps P->sighand->count. If P exec, it will
notice sighand->count != 1 and switch to another ->sighand.

Oleg.