lkml.org 
[lkml]   [2012]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Q: cgroup: Questions about possible issues in cgroup locking
    Oleg Nesterov (oleg@redhat.com) wrote:
    > On 01/18, Mandeep Singh Baines wrote:
    > >
    > > Oleg Nesterov (oleg@redhat.com) wrote:
    > > > On 01/13, Mandeep Singh Baines wrote:
    > > > >
    > > > > Case 3: g is some other thread
    > > > >
    > > > > In this case, g MUST be current
    > > >
    > > > Why? This is not true.
    > >
    > > Here is my thinking:
    > >
    > > The terminating condition, t != g, assumes that you can get back to
    > > g. If g is unhashed, there is no guarantee you'll ever get back to it.
    > > Holding a reference does not prevent unhashing.
    > >
    > > for_each_process avoids unhashing by starting and ending at init_task
    > > (which can never be unhashed).
    >
    > Yes,
    >
    > > As you pointed out a while back, this doesn't work for:
    > >
    > > do_each_thread(g, t){
    > > do_something(t);
    > > } while_each_thread(g, t)
    > >
    > > because g can be unhashed.
    > >
    > > However, you should be able to use while_each_thread if you are current.
    > > Being current would prevent 'g' from being unhashed.
    >
    > Ah, I misunderstood you. Yes, sure.
    >
    > > other than, do_each_thread/while_each_thread, all other callers
    > > of while_each_thread() are starting at current. Otherwise, how do
    > > you guarantee that it terminates.
    >
    > Hm, still can't understand...
    >

    On second thought. I think I've made some incorrect assumptions.

    > > I see at least one example, coredump_wait() that uses while_each_thread
    > > starting at current. I didn't find any cases where while_each_thread
    > > starts anywhere other than current or group_leader.
    >
    > Probably you meant zap_threads/zap_process, not coredump_wait?
    >
    > zap_process() is fine, we hold ->siglock. But zap_threads does _not_

    Ah. Missed the siglock.

    > start at current, may be you misread the g == tsk->group_leader check
    > in the main for_each_process() loop ? But most probably we simply
    > misunderstand each other a bit, see below.
    >
    > However it starts at ->group_leader, so it won't suffer if we restrict
    > the lockless while_each_thread_rcu().
    >
    > > > But I guess this doesn't matter, I think I am
    > > > starting to understand why our discussion was a bit confusing.
    > > >
    > > > The problem is _not_ exec/de_thread by itself. The problem is that
    > > > while_each_thread(g, t) can race with removing 'g' from the list.
    > > > IOW, list_for_each_rcu-like things assume that the head of the list
    > > > is "stable" but this is not true.
    > > >
    > > > And note that de_thread() does not matter in this sense, only
    > > > __unhash_process()->list_del_rcu(&p->thread_group) does matter.
    > > >
    > > > Now. If 'g' is the group leader, we should only worry about exec,
    > > > otherwise it can't disappear before other threads.
    > > >
    > > > But if it is not the group leader, it can simply exit and
    > > > while_each_thread(g, t) can never stop by the same reason.
    > > >
    > >
    > > I think we are on the same page. Your explanation is consistent with
    > > my understanding.
    > >
    > > Some other thoughts:
    > >
    > > I suspect that other than do_each_thread()/while_each_thread() or
    > > for_each_thread()/while_each_thread() where 'g' is the group_leader,
    > > 'g' MUST be current. So the only cases to consider are:
    >
    > I didn't try to grep, but I do not know any example of the lockless
    > while_each_thread() which starts at current.
    >

    Did a quick scan of all the while_each_thread()s under rcu_read_lock
    and couldn't find one that starts at current.

    > I guess this is the source of confusion.
    >
    > > > I'll try to recheck my thinking once again, what do you think? Anything
    > > > else we could miss?
    > >
    > > Yeah, the ->group_leader solution seems the most promising. It seems
    > > correct (ignoring barriers) and should work for all supported cases:
    > >
    > > 1) when g is group_leader
    > > 2) when g is current
    >
    > OK, thanks.
    >
    > I'll try to investigate if we can remove
    >
    > leader->group_leader = tsk;
    >
    > from de_thread(). In fact I already thought about this change a long
    > ago without any connection to while_each_thread(). This assignment
    > looks "assymetrical" compared to other threads we kill. But we did
    > have a reason (reasons?). Hopefully, the only really important reason
    > was already removed by 087806b1.
    >

    Ah. So the leader->group_leader may have been necessary earlier in order
    to prevent two tasks, old leader and new leader from both returning true
    for thread_group_leader(tsk).

    Regards,
    Mandeep

    > Oleg.
    >


    \
     
     \ /
      Last update: 2012-01-19 19:21    [W:3.055 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site