lkml.org 
[lkml]   [2010]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 06/14] signal: use GROUP_STOP_PENDING to avoid stopping multiple times for a single group stop
    Hello, Oleg.

    On 11/26/2010 06:59 PM, Oleg Nesterov wrote:
    > I am stucked at this point ;)

    :-)

    > On 11/26, Tejun Heo wrote:
    >> Currently task->signal->group_stop_count is used to decide whether to
    >> stop for group stop. However, if there is a task in the group which
    >> is taking a long time to stop, other tasks which are continued by
    >> ptrace would repeatedly stop for the same group stop until the group
    >> stop is complete.
    >
    > Yes. but the tracee won't abuse ->group_stop_count, this was fixed
    > by the previous patch.

    Yes.

    > But, otoh, what if debugger resumes the tracee when the group stop
    > was completed by other sub-threads ?

    Well, then the tracee continues. What this patch does is making each
    task in a group to stop once for a single group stop instance. If
    ptracer decides to resume the tracee (w/o sending SIGCONT, that is),
    then it can do so and the tracee won't stop for the same group stop
    again.

    > The tracee will run with GROUP_STOP_PENDING set. ->group_stop_count
    > is zero. If this tracee recieves a signal (or spurious TIF_SIGPENDING),
    > suddenly it will notice GROUP_STOP_PENDING and report the stop to
    > debugger.

    Yeah, of course. That's the tracee participating in the group stop.
    Oh, the tracee _should_ always have TIF_SIGPENDING set or be
    guaranteed to run get_signal_to_deliver(). I think there are traced
    points where that is not true. We probably need to set TIF_SIGPENDING
    together with GROUP_STOP_PENDING.

    > This looks a bit strange. OK, perhaps it makes sense to report the
    > stop to "ack" the group stop which wasn't acked in ptrace_stop().
    > Or, if it was untraced after resume, it makes sense to "silently"
    > stop as well.
    >
    > But, in this case it shouldn't wait until signal_pending() is true?

    Yeap, thanks a lot for catching that one. :-)

    >> @@ -1742,8 +1745,8 @@ static int do_signal_stop(int signr)
    >> struct signal_struct *sig = current->signal;
    >> int notify = 0;
    >>
    >> - if (!sig->group_stop_count) {
    >> - unsigned int gstop = GROUP_STOP_CONSUME;
    >> + if (!(current->group_stop & GROUP_STOP_PENDING)) {
    >> + unsigned int gstop = GROUP_STOP_PENDING | GROUP_STOP_CONSUME;
    >> struct task_struct *t;
    >
    > Hmm. This means, the ptraced task can initiate the group stop
    > while it is already in progress...
    >
    > Debugger can constantly resume a tracee while the group stop
    > is not finished. Finally this tracee can dequeue SIGSTOP without
    > GROUP_STOP_PENDING.
    >
    > At first glance, nothing bad can happen, but I am not sure.
    > We can have other ptraced threads which were resumed after
    > ptrace_stop()/do_signal_stop().

    Hmmm.... right. I think it is better to test for GROUP_STOP_PENDING
    there. That happens on delivery of a new stop signal, so
    semantic-wise, it's correct. Given the statelessness of group stop
    across STOP/CONT attempts, I think it should be okay. I'll think
    about it more.

    >> This will change with future patches.
    >
    > Yes. I tried to study this series patch-by-patch. I think I should
    > read the whole series to really understand the intermediate changes.
    > I'll try to return on Monday.
    >
    > Cough. I didn't expect I forgot this code that much ;)

    Heh, I thought adding transparent/nestable ptrace attach would take me
    several days; instead, understanding the code and producing this
    patchset took me two weeks filled with swearing. This is a truly
    hairy piece of code. :-)

    Thanks a lot for reviewing.

    --
    tejun


    \
     
     \ /
      Last update: 2010-11-26 19:43    [W:0.094 / U:32.312 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site