lkml.org 
[lkml]   [2011]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/4] threadgroup: extend threadgroup_lock() to cover exit and exec
    Also sorry for my late reply. Some thoughts.

    On Sun, Sep 18, 2011 at 07:37:23PM +0200, Oleg Nesterov wrote:
    > Hello,
    >
    > Sorry for the late reply.
    >
    > Of course I am in no position to ack the changes in this code, I do not
    > fell I understand it enough. But afaics this series is fine.
    >
    > A couple of questions.
    >
    > On 09/05, Tejun Heo wrote:
    > >
    > > For exec, threadgroup_[un]lock() are updated to also grab and release
    > > cred_guard_mutex.
    >
    > OK, this means that we do not need
    >
    > cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch
    > http://marc.info/?l=linux-mm-commits&m=131491135428326&w=2
    >
    > Ben, what do you think?

    Hmm. So now threadgroup_lock() protects the ->thread_group list in all
    situations (exit protected by the diff below, and exec protected by the
    cred_guard_mutex)?

    I'm not sure if I like the pattern of "you can take either these
    high-level locks or take this spinlock to protect the list". But it
    seems safe enough, so it's fine by me.

    Just to be clear, I think we still need the "double-check and possibly
    try again" behaviour, right?

    Considering that the cred_guard_mutex critical section is hard to find
    (it is unlocked in install_exec_creds, which is defined in fs/exec.c and
    called in fs/binfmt_*.c) I would probably like to see an assert of
    mutex_is_locked(cred_guard_mutex) in de_thread, with this change.

    Thanks for working on this, Tejun.

    -- Ben

    >
    > > With this change, threadgroup_lock() guarantees that the target
    > > threadgroup will remain stable - no new task will be added, no new
    > > PF_EXITING will be set and exec won't happen.
    >
    > To me, this is the only "contradictory" change,
    >
    > > --- a/kernel/exit.c
    > > +++ b/kernel/exit.c
    > > @@ -936,6 +936,12 @@ NORET_TYPE void do_exit(long code)
    > > schedule();
    > > }
    > >
    > > + /*
    > > + * @tsk's threadgroup is going through changes - lock out users
    > > + * which expect stable threadgroup.
    > > + */
    > > + threadgroup_change_begin(tsk);
    > > +
    > > exit_irq_thread();
    > >
    > > exit_signals(tsk); /* sets PF_EXITING */
    > > @@ -1018,10 +1024,6 @@ NORET_TYPE void do_exit(long code)
    > > kfree(current->pi_state_cache);
    > > #endif
    > > /*
    > > - * Make sure we are holding no locks:
    > > - */
    > > - debug_check_no_locks_held(tsk);
    > > - /*
    > > * We can do this unlocked here. The futex code uses this flag
    > > * just to verify whether the pi state cleanup has been done
    > > * or not. In the worst case it loops once more.
    > > @@ -1039,6 +1041,12 @@ NORET_TYPE void do_exit(long code)
    > > preempt_disable();
    > > exit_rcu();
    > >
    > > + /*
    > > + * Release threadgroup and make sure we are holding no locks.
    > > + */
    > > + threadgroup_change_done(tsk);
    >
    > I am wondering, can't we narrow the scope of threadgroup_change_begin/done
    > in do_exit() path?
    >
    > The code after 4/4 still has to check PF_EXITING, this is correct. And yes,
    > with this patch PF_EXITING becomes stable under ->group_rwsem. But, it seems,
    > we do not really need this?
    >
    > I mean, can't we change cgroup_exit() to do threadgroup_change_begin/done
    > instead? We do not really care about PF_EXITING, we only need to ensure that
    > we can't race with cgroup_exit(), right?

    That sounds right to me. After all, in the fork bailout path where
    cgroup_exit is also called is just before the lock is dropped.

    >
    > Say, cgroup_attach_proc() does
    >
    > do {
    > if (tsk->flags & PF_EXITING)
    > continue;
    >
    > flex_array_put_ptr(group, tsk);
    > } while_each_thread();
    >
    > Yes, this tsk can call do_exit() and set PF_EXITING right after the check
    > but this is fine. The only guarantee we need is: if it has already called
    > cgroup_exit() we can not miss PF_EXITING, and if cgroup_exit() takes the
    > same sem this should be true. And, otoh, if we do not see PF_EXITING then
    > we can not race with cgroup_exit(), it should block on ->group_rwsem hold
    > by us.

    Right.

    >
    > If I am right, afaics the only change 4/4 needs is that it should not add
    > WARN_ON_ONCE(tsk->flags & PF_EXITING) into cgroup_task_migrate().
    >
    > What do you think?
    >
    > Oleg.
    >
    >

    This bit looks suspicious (but only stylistically):

    retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
    - BUG_ON(retval != 0 && retval != -ESRCH);
    + BUG_ON(retval != 0);

    Is this also the case for the other callsite to cgroup_task_migrate? If
    so, maybe change cgroup_task_migrate to return void, and have the BUG_ON
    inside of it.

    Cheers,
    Ben



    \
     
     \ /
      Last update: 2011-10-08 20:47    [W:0.031 / U:122.380 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site