lkml.org 
[lkml]   [2009]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.



    \
     
     \ /
      Last update: 2009-09-03 19:49    [W:0.022 / U:0.052 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site