On 03/18, Roland McGrath wrote: > > It's OK to change the tracehook definition. I did this on the new git > branch tracehook, then utrace branch commit 7b0be6e4 merges that and uses it.
Roland, I think it better to change tracehook definition more, please see below. > This drops all the JCTL bit kludgery and utrace_report_jctl just backs out > of TASK_STOPPED before dropping the siglock in the first place. I think > the bookkeeping covers all the angles, but please check it in the new code. Heh. I was thinking about the very similar change. But I have problems with tracehook_notify_jctl(). Please find the patch below, on top of your changes. At the cost of one additional ->group_stop_count != 0 in do_signal_stop(), we can avoid playing with state/group_stop_count/flags twice. But, with or without this patch we have a small problem: we can wrongly send SIGCHLD twice. I'll write a separate email. > Also please verify if you think all ->stopped bookkeeping is bulletproof > now. I fiddled utrace_get_signal() a little because I wasn't quite sure > that all possibly paths there after TASK_STOPPED were resetting it. Will do. I didn't study the signal part of utrace yet. > I want to post it > to LKML in the next day or two so it has aired before the 2.6.30 merge > window. Yes! I think it should be posted really soon. BTW. exit_signals() calls tracehook_notify_jctl(why => CLD_STOPPED), could you confirm this is right? ------------------------------------------------------------------------- [PATCH] simplify do_signal_stop() && utrace_report_jctl() interaction do_signal_stop() can call utrace_report_jctl() before decrementing ->group_stop_count and setting TASK_STOPPED/SIGNAL_STOP_STOPPED. This allow to greatly simplify utrace_report_jctl() and avoid playing with group-stop bookkeeping twice. Signed-off-by: Oleg Nesterov <o...@redhat.com> signal.c | 29 +++++++++++------------------ utrace.c | 20 -------------------- 2 files changed, 11 insertions(+), 38 deletions(-) --- xxx/kernel/signal.c~JCTL_SIMPLIFY 2009-03-18 14:50:06.000000000 +0100 +++ xxx/kernel/signal.c 2009-03-18 18:20:35.000000000 +0100 @@ -1638,16 +1638,9 @@ void ptrace_notify(int exit_code) static int do_signal_stop(int signr) { struct signal_struct *sig = current->signal; - int stop_count; int notify; - if (sig->group_stop_count > 0) { - /* - * There is a group stop in progress. We don't need to - * start another one. - */ - stop_count = --sig->group_stop_count; - } else { + if (!sig->group_stop_count) { struct task_struct *t; if (!likely(sig->flags & SIGNAL_STOP_DEQUEUED) || @@ -1659,7 +1652,7 @@ static int do_signal_stop(int signr) */ sig->group_exit_code = signr; - stop_count = 0; + sig->group_stop_count = 1; for (t = next_thread(current); t != current; t = next_thread(t)) /* * Setting state to TASK_STOPPED for a group @@ -1668,25 +1661,25 @@ static int do_signal_stop(int signr) */ if (!(t->flags & PF_EXITING) && !task_is_stopped_or_traced(t)) { - stop_count++; + sig->group_stop_count++; signal_wake_up(t, 0); } - sig->group_stop_count = stop_count; } - if (stop_count == 0) - sig->flags = SIGNAL_STOP_STOPPED; - current->exit_code = sig->group_exit_code; - __set_current_state(TASK_STOPPED); - /* * If there are no other threads in the group, or if there is * a group stop in progress and we are the last to stop, * report to the parent. When ptraced, every thread reports itself. */ - notify = tracehook_notify_jctl(stop_count == 0 ? CLD_STOPPED : 0, - CLD_STOPPED); + notify = sig->group_stop_count == 1 ? CLD_STOPPED : 0; + notify = tracehook_notify_jctl(notify, CLD_STOPPED); + if (sig->group_stop_count) { + if (!--sig->group_stop_count) + sig->flags = SIGNAL_STOP_STOPPED; + current->exit_code = sig->group_exit_code; + __set_current_state(TASK_STOPPED); + } spin_unlock_irq(¤t->sighand->siglock); if (notify) { --- xxx/kernel/utrace.c~JCTL_SIMPLIFY 2009-03-18 14:50:06.000000000 +0100 +++ xxx/kernel/utrace.c 2009-03-18 18:23:01.000000000 +0100 @@ -1618,18 +1618,7 @@ void utrace_report_jctl(int notify, int struct task_struct *task = current; struct utrace *utrace = task_utrace_struct(task); INIT_REPORT(report); - bool stop = task_is_stopped(task); - /* - * We have to come out of TASK_STOPPED in case the event report - * hooks might block. Since we held the siglock throughout, it's - * as if we were never in TASK_STOPPED yet at all. - */ - if (stop) { - __set_current_state(TASK_RUNNING); - task->signal->flags &= ~SIGNAL_STOP_STOPPED; - ++task->signal->group_stop_count; - } spin_unlock_irq(&task->sighand->siglock); /* @@ -1654,16 +1643,7 @@ void utrace_report_jctl(int notify, int REPORT(task, utrace, &report, UTRACE_EVENT(JCTL), report_jctl, what, notify); - /* - * Retake the lock, and go back into TASK_STOPPED - * unless the stop was just cleared. - */ spin_lock_irq(&task->sighand->siglock); - if (stop && task->signal->group_stop_count > 0) { - __set_current_state(TASK_STOPPED); - if (--task->signal->group_stop_count == 0) - task->signal->flags |= SIGNAL_STOP_STOPPED; - } } /*