[lkml]   [2006]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH rc1-mm] de_thread: fix deadlockable process addition
    Oleg Nesterov <> writes:

    > On 04/08, Eric W. Biederman wrote:
    >> Agreed. That is ugly.
    > Yes, I agree also.
    >> -#define thread_group_leader(p) (p->pid == p->tgid)
    >> +#define thread_group_leader(p) (p == p->group_leader)
    >> ...
    >> - leader->group_leader = leader;
    >> + leader->group_leader = current;
    > I thought about similar change too, but I am unsure about
    > release_task(old_leader)->proc_flush_task() path (because
    > I don't understand this code).

    proc_flush_task() is purely an optimization to kill the
    proc dcache entries when a process exits. So we don't
    plug up the dcache with old proc entries.

    All it looks at currently and pid an tgid.

    So proc_flush_task() should not be a non-issue.

    proc_pid_unhash() in -linus is a little more
    serious but it only unhashes things.

    > This change can confuse next_tid(), but this is minor.
    > I don't see other problems.


    > However, I think we can do something better instead of
    > attach_pid(current)/detach_pid(leader):
    > void exec_pid(task_t *old, task_t * new, enum pid_type type)
    > {
    > new->pids[type].pid = old->pids[type].pid;
    > hlist_replace_rcu(&old->pids[type].node, &new->pids[type].node);
    > old->pids[type].pid = NULL;
    > }
    > So de_thread() can do
    > exec_pid(leader, current, PIDTYPE_PGID);
    > exec_pid(leader, current, PIDTYPE_SID);
    > This allows us to iterate over pgrp/session lockless without
    > seeing the same task twice, btw. But may be it is just unneeded
    > complication.

    I think it may be worthwhile. Currently we can't do any process
    group or session traversal lockless so it isn't worth looking
    at until we get the bug fix handled.

    Ultimately I think I would like PGID and SID to live in ->signal
    in which case we would not need to touch them at all.

    >> This requires changing the leaders parents
    >> current->parent = current->real_parent = leader->real_parent;
    >> - leader->parent = leader->real_parent = child_reaper;
    >> + leader->parent = leader->real_parent = current;
    >> current->group_leader = current;
    > I don't understand why do we need this change.

    I was just trying to come as close as I could to normal
    thread semantics. The fewer special cases in de_thread,
    the fewer problems it is.

    > Actually, I think leader doesn't need reparenting at all.
    > ptrace_unlink(leader) already restored leader->parent = ->real_parent
    > and ->sibling. So I think we can do (for review only, should go in a
    > separate patch) this:

    Duh. All processes in a thread group share the same real_parent.
    It looks like the only practical thing we accomplish
    with remove_parent/add_parent is to change our place on
    our parents list of children.

    Since ptrace_link and ptrace_unlink already does this we don't have a
    guaranteed order on that list, so skipping the order change
    should definitely be safe.

    This means your patch doesn't go far enough. We should be
    able to kill all of the parent list manipulation in
    de_thread. Doing reduces the places that assign
    real_parent to just fork and exit.

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2006-04-08 18:14    [W:0.034 / U:0.680 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site