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;
- }
}
/*
