lkml.org 
[lkml]   [2011]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/8] job control: Fix ptracer wait(2) hang and explain notask_error clearing
    On 03/08, Tejun Heo wrote:
    >
    > static void *nooper(void *arg)
    > {
    > pause();
    > return NULL;
    > }
    >
    > int main(void)
    > {
    > const struct timespec ts1s = { .tv_sec = 1 };
    > pid_t tracee, tracer;
    > siginfo_t si;
    >
    > tracee = fork();
    > if (tracee == 0) {
    > pthread_t thr;
    >
    > pthread_create(&thr, NULL, nooper, NULL);
    > nanosleep(&ts1s, NULL);
    > printf("tracee exiting\n");
    > pthread_exit(NULL); /* let subthread run */
    > }
    >
    > tracer = fork();
    > if (tracer == 0) {
    > ptrace(PTRACE_ATTACH, tracee, NULL, NULL);
    > while (1) {
    > if (waitid(P_PID, tracee, &si, WSTOPPED) < 0) {
    > perror("waitid");
    > break;
    > }
    > ptrace(PTRACE_CONT, tracee, NULL,
    > (void *)(long)si.si_status);
    > }
    > return 0;
    > }
    >
    > waitid(P_PID, tracer, &si, WEXITED);
    > kill(tracee, SIGKILL);
    > return 0;
    > }
    >
    > Before the patch, after the tracee becomes a zombie, the tracer's
    > waitid(WSTOPPED) never returns and the program doesn't terminate.

    Yes. The program should terminate after nooper() exits. Agreed, this
    looks strange.

    > After the patch, tracee exiting triggers waitid() to fail.
    >
    > tracee exiting
    > waitid: No child processes

    OK, but

    > @@ -1550,17 +1550,41 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
    > return 0;
    > }
    >
    > - /*
    > - * We don't reap group leaders with subthreads.
    > - */
    > - if (p->exit_state == EXIT_ZOMBIE && !delay_group_leader(p))
    > - return wait_task_zombie(wo, p);
    > + /* slay zombie? */
    > + if (p->exit_state == EXIT_ZOMBIE) {
    > + /* we don't reap group leaders with subthreads */
    > + if (!delay_group_leader(p))
    > + return wait_task_zombie(wo, p);
    >
    > - /*
    > - * It's stopped or running now, so it might
    > - * later continue, exit, or stop again.
    > - */
    > - wo->notask_error = 0;
    > + /*
    > + * Allow access to stopped/continued state via zombie by
    > + * falling through. Clearing of notask_error is complex.
    > + *
    > + * When !@ptrace:
    > + *
    > + * If WEXITED is set, notask_error should naturally be
    > + * cleared. If not, subset of WSTOPPED|WCONTINUED is set,
    > + * so, if there are live subthreads, there are events to
    > + * wait for. If all subthreads are dead, it's still safe
    > + * to clear - this function will be called again in finite
    > + * amount time once all the subthreads are released and
    > + * will then return without clearing.
    > + *
    > + * When @ptrace:
    > + *
    > + * Stopped state is per-task and thus can't change once the
    > + * target task dies, so notask_error should be cleared only
    > + * if WCONTINUED is set.
    > + */
    > + if (likely(!ptrace) || (wo->wo_flags & WCONTINUED))
    > + wo->notask_error = 0;

    I don't understand this part. Suppose that this task is not traced and
    its real parent does do_wait(WEXITED). We shouldn't return -ECHLD in
    this case (EXIT_ZOMBIE && delay_group_leader()).

    If the task is traced and debugger does do_wait(WEXITED) we should not
    return -ECHLD too.

    So I think this patch is wrong in its current state. Hmm, I just noticed
    the comment says "If WEXITED is set, notask_error should ...", but this
    is not what the code does?





    But the main problem is, I do not think do_wait() should block in this
    case, and thus I am starting to think this patch is not "complete".

    I mean, we are going to add the user-visible change, and I agree the
    current behaviour looks strange. But if we change this code, perhaps
    the tracer should ignore delay_group_leader() at all (unless we are
    going to do wait_task_zombie) ?

    Your test-case could use waitid(WEXITED) instead WSTOPPED with the same
    result, it should hang. Why it hangs? The tracee is dead, we can't do
    ptrace(PTRACE_DETACH), and we can do nothing until other threads exit.
    This looks equally strange.

    IOW. Assuming that ptrace == T and WEXITED is set, perhaps we should
    do something like this pseudo-code

    if (p->exit_state == EXIT_ZOMBIE) {
    if (!delay_group_leader(p))
    return wait_task_zombie(wo, p);
    ptrace_unlink();
    wait_task_zombie(WNOWAIT);
    }
    However. This is another user-visible change, we need another discussion
    even if I am right. In particular, it is not clear what should we do
    if parent == real_parent. And probably this can confuse gdb, but iirc
    gdb already have the problems with the dead leader anyway.

    Oleg.



    \
     
     \ /
      Last update: 2011-03-21 16:31    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean