Oleg Nesterov (oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org) wrote:

On 01/18, Mandeep Singh Baines wrote:

quoted

Oleg Nesterov (oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org) wrote:

quoted

On 01/13, Mandeep Singh Baines wrote:

quoted

Case 3: g is some other thread

In this case, g MUST be current

Why? This is not true.

Here is my thinking:

The terminating condition, t != g, assumes that you can get back to
g. If g is unhashed, there is no guarantee you'll ever get back to it.
Holding a reference does not prevent unhashing.

for_each_process avoids unhashing by starting and ending at init_task
(which can never be unhashed).

Yes,

quoted

As you pointed out a while back, this doesn't work for:

do_each_thread(g, t){
  do_something(t);
} while_each_thread(g, t)

because g can be unhashed.

However, you should be able to use while_each_thread if you are current.
Being current would prevent 'g' from being unhashed.

Ah, I misunderstood you. Yes, sure.

quoted

other than, do_each_thread/while_each_thread, all other callers
of while_each_thread() are starting at current. Otherwise, how do
you guarantee that it terminates.

Hm, still can't understand...

On second thought. I think I've made some incorrect assumptions.

quoted

I see at least one example, coredump_wait() that uses while_each_thread
starting at current. I didn't find any cases where while_each_thread
starts anywhere other than current or group_leader.

Probably you meant zap_threads/zap_process, not coredump_wait?

zap_process() is fine, we hold ->siglock. But zap_threads does _not_

Ah. Missed the siglock.

start at current, may be you misread the g == tsk->group_leader check
in the main for_each_process() loop ? But most probably we simply
misunderstand each other a bit, see below.

However it starts at ->group_leader, so it won't suffer if we restrict
the lockless while_each_thread_rcu().

quoted

quoted

But I guess this doesn't matter, I think I am
starting to understand why our discussion was a bit confusing.

The problem is _not_ exec/de_thread by itself. The problem is that
while_each_thread(g, t) can race with removing 'g' from the list.
IOW, list_for_each_rcu-like things assume that the head of the list
is "stable" but this is not true.

And note that de_thread() does not matter in this sense, only
__unhash_process()->list_del_rcu(&p->thread_group) does matter.

Now. If 'g' is the group leader, we should only worry about exec,
otherwise it can't disappear before other threads.

But if it is not the group leader, it can simply exit and
while_each_thread(g, t) can never stop by the same reason.

I think we are on the same page. Your explanation is consistent with
my understanding.

Some other thoughts:

I suspect that other than do_each_thread()/while_each_thread() or
for_each_thread()/while_each_thread() where 'g' is the group_leader,
'g' MUST be current. So the only cases to consider are:

I didn't try to grep, but I do not know any example of the lockless
while_each_thread() which starts at current.

Did a quick scan of all the while_each_thread()s under rcu_read_lock
and couldn't find one that starts at current.

I guess this is the source of confusion.

quoted

quoted

I'll try to recheck my thinking once again, what do you think? Anything
else we could miss?

Yeah, the ->group_leader solution seems the most promising. It seems
correct (ignoring barriers) and should work for all supported cases:

1) when g is group_leader
2) when g is current

OK, thanks.

I'll try to investigate if we can remove

	leader->group_leader = tsk;

from de_thread(). In fact I already thought about this change a long
ago without any connection to while_each_thread(). This assignment
looks "assymetrical" compared to other threads we kill. But we did
have a reason (reasons?). Hopefully, the only really important reason
was already removed by 087806b1.

Ah. So the leader->group_leader may have been necessary earlier in order
to prevent two tasks, old leader and new leader from both returning true
for thread_group_leader(tsk).

Regards,
Mandeep

Oleg.