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    [W:0.576 / U:0.292 seconds]
©2003-2014 Jasper Spaans. Advertise on this site