Messages in this thread | | | Date | Tue, 22 Jun 2010 16:34:02 +0200 | From | Oleg Nesterov <> | Subject | Re: while_each_thread() under rcu_read_lock() is broken? |
| |
On 06/21, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@redhat.com> writes: > > >> If > >> that's so, then just changing it to avoid the situation seems like it > >> would be less invasive overall. > > > > How? We should change ->group_leader uner write_lock_irq(tasklist), > > synchronize_rcu() is not an option. We can't do call_rcu(release_task), > > we can't take tasklist for writing in the softirq context. But even > > if we could, this can't help in fact or I missed something. > > We already do: call_rcu(&p->rcu, delayed_put_task_struct); in release_task. > We don't call release_task until after we have removed it as leader and > dropped the write lock.
Yes. I meant that while this is needed to ensure that next_thread/etc returns the rcu-safe data, this (or more rcu_call's) can help to fix while_each_thread.
I think I was unclear. de_thread() changes ->group_leader, but this does not matter at all. I mentioned this only because we discussed the possibility to check ->group_leader in while_each_thread.
What does matter is the single line in __unhash_process()
list_del_rcu(&p->thread_group);
this breaks while_each_thread().
IOW. Why list_for_each_rcu(head) actually works? It works because this head itself can't be removed from list.
while_each_thread(g, t) is almost equal to list_for_each_rcu() and it can only work if g can't be removed from list (OK, strictly speaking until other sub-threads go away, but this doesn't matter).
However, g can be removed if a) it is not ->group_leader and exits, or b) its subthread execs.
> At first glance it sounds like the group leader is safe as a stopping > point for a rcu while_each_thread, and I expect the fact that > de_thread takes everything down to a single thread, could have nice > properties here. If pid_alive were only to fail on the group leader > when de_thread is called I think we could legitimately say that an event > we won't worry about. It is close enough to a new thread being created > anyway.
Not sure I understand exactly... I mean, I don't understand whether you agree or not with the fix which adds pid_alive() check into next_thread_careful().
Oleg.
| |