lkml.org 
[lkml]   [2010]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] [PATCH 1/2] cgroups: read-write lock CLONE_THREAD forking per threadgroup
    On 01/17, Ben Blum wrote:
    >
    > On Tue, Jan 05, 2010 at 07:53:30PM +0100, Oleg Nesterov wrote:
    > >
    > > I don't understand how this can close the race with de_thread().
    > > ...
    >
    > the race with the sighand is handled in the next patch, in attach_proc,
    > not in this function.

    OK. I didn't verify this, the patches don't apply to 2.6.32-rc, but this
    doesn't matter. Please see below.

    > > > + /* now try to find a sighand */
    > > > + if (likely(tsk->sighand)) {
    > > > + sighand = tsk->sighand;
    > > > + } else {
    > > > + sighand = ERR_PTR(-ESRCH);
    > > > + /*
    > > > + * tsk is exiting; try to find another thread in the group
    > > > + * whose sighand pointer is still alive.
    > > > + */
    > > > + list_for_each_entry_rcu(p, &tsk->thread_group, thread_group) {
    > > > + if (p->sighand) {
    > > > + sighand = tsk->sighand;
    > >
    > > can't understand this "else {}" code... We hold tasklist, if the group
    > > leader is dead (->sighand == NULL), then the whole thread group is
    > > dead.
    > >
    > > Even if we had another thread with ->sighand != NULL, what is the point
    > > of "if (unlikely(!thread_group_leader(tsk)))" check then?
    >
    > doesn't the group leader stay on the threadgroup list even when it dies?
    > sighand can be null if the group leader has exited, but other threads
    > are still running.

    No, leader->sighand != NULL until all threads have exited.


    Ben, I'd suggest you to redo these patches even if they are correct.
    ->sighand is not the right place for the mutex/locking

    - it is per CLONE_SIGHAND, not per process

    - we have to avoid the nasty and hard-to-test races with exec

    - we have to play with sighand->count and I really dislike this.
    this ->count is not just a reference counter, look at
    unshare_sighand(). Yes, this is fake, but still.

    Please use ->signal instead. By the lucky coincidence the lifetime rules
    for (greatly misnamed) signal_struct were changed recently in -mm.

    With the recent changes, it is always safe to use task->signal. It can't
    be changed, can't go away, no need to bump the counter, no races, etc.

    What do you think?

    Oleg.



    \
     
     \ /
      Last update: 2010-03-22 11:27    [W:0.052 / U:29.224 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site