Re: [PATCH rc5-mm] pids: kill PIDTYPE_TGID

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@tv-sign.ru> writes:
>>
>> > depends on
>> >
>> > 	pidhash-dont-count-idle-threads.patch
>> > 	pidhash-kill-switch_exec_pids.patch
>> >
>> > otherwise (I think) it is orthogonal to all tref/proc changes.
>>
>> You also depend on your recent change to call __unhash_process
>> under the sighand lock.  Since the ->tgrp is protected by
>> the sighand lock.
>
> Actually now I think it depends only on yours
> 	pidhash-kill-switch_exec_pids.patch
>
> This change (call __unhash_process under the sighand lock) does not
> touch __unhash_process() itself, it only moves the callsite. So this
> patch can go before or after this change. However it needs other
> patches from -mm to avoid rejects.

Agreed.  I misread the code and didn't see that we had the tasklist_lock where
you did the list_add.  

>> > This patch kills PIDTYPE_TGID pid_type thus saving one hash table
>> > in kernel/pid.c and speeding up subthreads create/destroy a bit.
>> > It is also a preparation for the further tref/pids rework.
>> >
>> > This patch adds 'struct list_head tgrp' to 'struct task_struct'
>> > instead. Note that ->tgrp need not to be rcu safe.
>>
>> Is there a reason for this?  I think at least proc could easily
>> take advantage of an rcu safe implementation.
>>
>> Hmm.  At the moment proc is only taking the tasklist_lock during
>> traversal so we may have a problem with the list_add anyway.
>
> tasklist or ->sighand is enough to do threads traversal. Yes,
> adding _rcu suffix allows us to do it under rcu_read_lock().
> However the thread group will not be stable during this traversal,
> we can miss some newly created thread or hit an already unhashed
> one.

Right so if what we are doing is needs to be atomic we can't drop
the lock.  Which makes sense.

> Note also, that this loop:
> 
> 	t = task;
> 	do {
> 		...
> 		t = next_thread(t);
> 	} while (t != task);
>
> requires that task->tgrp is stable (I mean, 'task' must not be
> unhashed during this loop). And I can't see how this is possible
> unless we take ->sighand or tasklist_lock or task == current.
>
> Eric, I know nothing about proc/, so the question is: do you want
> me to add _rcu right now, or do you think it's better to do this
> later?

Let's add the manipulations right now, that way we have the option
to convert the users that can take advantage of it.

> grepping for next_thread in proc/ I have the feeling that we can
> convert the code from:
>
> 	read_lock(tasklist);
> 	if (pid_alive(task)) {
> 		do ... while (t != task);
> 	}
> 	read_unlock(tasklist);
>
> to:
>
> 	rcu_read_lock();
> 	if (lock_task_sighand(task)) {
> 		...
> 		unlock_task_sighand(task);
> 	}
> 	rcu_read_unlock();
>
> But again, I don't understand this code.

For proc we don't even need the lock_task_sighand.  It is purely best
effort.  The more we can remove lock contention from proc the harder it
will be for a user space application to trigger long lock hold
times.

>> Also could we name the member not ->tgrp but ->threads?
>> I keep half expecting tgrp to be a number, and have a hard
>> time not mispelling it tpgrp.
>
> Agreed. I also started with 'threads' name, but we already have
> 'struct thread_struct thread' in the task_struct. So may be
> we could name it thread_group? I am rather agnostic and have nothing
> against 'threads', will resend this patch after yours decision.

Ok.  Let's go with thread_group.  It isn't a frequently used field
so as long as the name doesn't get terribly long things will work,
and thread_group is still much shorter than it's current name :)

Eric