Messages in this thread | | | Date | Thu, 3 Sep 2009 19:41:06 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] core: allow setrlimit to non-current tasks |
| |
On 09/03, Jiri Slaby wrote: > > @@ -1240,6 +1240,7 @@ int setrlimit(struct task_struct *tsk, unsigned int resource, > struct rlimit *new_rlim) > { > struct rlimit *old_rlim; > + unsigned int needs_locking = !same_thread_group(tsk, current); > int retval;
Yes, thanks for doing this, imho this optimization is worthwhile.
But I'd suggest you to add this optimization in a separate patch because,
> + /* optimization: 'current' doesn't need locking, e.g. setrlimit */ > + if (needs_locking) { > + /* protect tsk->signal and tsk->sighand from disappearing */ > + read_lock(&tasklist_lock); > + if (!tsk->sighand) { > + retval = -ESRCH; > + goto unlock;
I should have mentioned this before, but it is not that simple.
Even if same_thread_group(tsk, current), we must not trust tsk->sighand, it can be NULL if our subthread is dead. (well, we need ->signal, not ->sighand but this doesn't matter because they disappear simultaneously).
Actually, perhaps same_thread_group() is not needed, perhaps it is enough to avoid tasklist in sys_setrlimit case. So, I think optimization should do:
retval = -ESRCH; if (tsk != current) { read_lock(&tasklist_lock); if (!tsk->sighand) goto unlock; }
unlock: if (tsk != current) read_unlock(&tasklist_lock);
Or, if we use same_thread_group(),
needs_locking = !same_thread_group(tsk, current);
if (!needs_locking) tsk = current; else { take tasklist, check ->sighand. }
Oleg.
| |