[PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[Oleg Nesterov]

The signal has no effect (but can provoke the unnecessary wakeup) if the
thread group is dying. Let's make this explicit and check SIGNAL_GROUP_EXIT
only once in handle_stop_signal() renamed to prepare_signal().

>From now the actual signal-delivery path doesn't need to take the special
SIGNAL_GROUP_EXIT case into account.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 25/kernel/signal.c~8_SS_CK_SGE	2008-03-09 20:21:02.000000000 +0300
+++ 25/kernel/signal.c	2008-03-09 21:18:19.000000000 +0300
@@ -564,16 +564,16 @@ static void do_notify_parent_cldstop(str
  * actual continuing for SIGCONT, but not the actual stopping for stop
  * signals.  The process stop is done as a signal action for SIG_DFL.
  */
-static void handle_stop_signal(int sig, struct task_struct *p)
+static int prepare_signal(int sig, struct task_struct *p)
 {
 	struct signal_struct *signal = p->signal;
 	struct task_struct *t;
 
-	if (signal->flags & SIGNAL_GROUP_EXIT)
+	if (unlikely(signal->flags & SIGNAL_GROUP_EXIT))
 		/*
 		 * The process is in the middle of dying already.
 		 */
-		return;
+		return 0;
 
 	if (sig_kernel_stop(sig)) {
 		/*
@@ -644,6 +644,8 @@ static void handle_stop_signal(int sig, 
 			signal->flags &= ~SIGNAL_STOP_DEQUEUED;
 		}
 	}
+
+	return 1;
 }
 
 /*
@@ -708,8 +710,7 @@ static void complete_signal(int sig, str
 	 * Found a killable thread.  If the signal will be fatal,
 	 * then start taking the whole group down immediately.
 	 */
-	if (sig_fatal(p, sig) && !(signal->flags & SIGNAL_GROUP_EXIT) &&
-	    !sigismember(&t->real_blocked, sig) &&
+	if (sig_fatal(p, sig) && !sigismember(&t->real_blocked, sig) &&
 	    (sig == SIGKILL || !(t->ptrace & PT_PTRACED))) {
 		/*
 		 * This signal will be fatal to the whole group.
@@ -753,7 +754,8 @@ static int send_signal(int sig, struct s
 	struct sigqueue *q;
 
 	assert_spin_locked(&t->sighand->siglock);
-	handle_stop_signal(sig, t);
+	if (!likely(prepare_signal(sig, t)))
+		return 0;
 
 	pending = group ? &t->signal->shared_pending : &t->pending;
 	/*
@@ -1247,9 +1249,10 @@ int send_sigqueue(struct sigqueue *q, st
 	if (!likely(lock_task_sighand(t, &flags)))
 		goto ret;
 
-	handle_stop_signal(sig, t);
-
 	ret = 1;
+	if (!likely(prepare_signal(sig, t)))
+		goto out;
+
 	if (sig_ignored(t, sig))
 		goto out;
 

Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[Roland McGrath]

I think your logic is sound.  But for paranoia's sake I would add
a BUG_ON(signal->flags & SIGNAL_GROUP_EXIT) in the group-fatal case.

I don't really like dropping the signal on the floor.  If the posting says
it succeeded (vs -ESRCH) then I'd like to see it appear later in the
pending set of the zombie thread, when we look at /proc/pid/status at exit
tracing or whatnot (and in core dumps).  Seeing those pending signal bits
is sometimes a useful clue about how something died in a strange scenario.


Thanks,
Roland

Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[Oleg Nesterov]

On 03/10, Roland McGrath wrote:
>
> I don't really like dropping the signal on the floor.

To clarify: my opinion is quite opposite, but it is only based on the
"personal feeling", I don't have any "strong" arguments. If you still
think we shouldn't do this, please nack this change.

> If the posting says
> it succeeded (vs -ESRCH) then I'd like to see it appear later in the
> pending set of the zombie thread, when we look at /proc/pid/status at exit
> tracing or whatnot (and in core dumps).  Seeing those pending signal bits
> is sometimes a useful clue about how something died in a strange scenario.

As for core dumps. Suppose that the task has already started do_coredump()
and another signal comes. Why should fill_prstatus() report this new signal?
This doesn't look exactly right for me. The coredump should try to reflect
the state of the task which it had when it was killed.

Actually, the same for zombies. If the task sits in Z state, one can look
at /proc/pid/status to see what signals it had when exited. To me, it looks
just better if it is visible that a zombie doesn "accept" the new signals,
because it is dead. (offtopic: currently the single-threaded exit doesn't
set SIGNAL_GROUP_EXIT. This doesn't matter now, but I think it would be
nice to be more consistent here).

Now, suppose that due to some kernel bug the running thread/process doesn't
want to die despite of SIGNAL_GROUP_EXIT. In that case another kill() may help
because of additional wakeup, but this will hide the problem which won't be
reported.

But again, I see your point, thanks.

Oleg.

Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[Roland McGrath]

There are arguments to be had about deciding that change to the behavior.
We can discuss it more if you like.  But that is rather different from
quietly rolling in your choices of behavior change to a "cleanup" patch.


Thanks,
Roland

Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[Oleg Nesterov]

Andrew, please drop these patches:

	signals-send_signal-factor-out-signal_group_exit-checks.patch
	signals-fold-sig_ignored-into-prepare_signal.patch
	signals-document-cld_continued-notification-mechanics.patch

(the last one refers to prepare_signal() introduced in the first one,
 and the comment doesn't match the canonical style)

On 03/11, Roland McGrath wrote:
>
> There are arguments to be had about deciding that change to the behavior.
> We can discuss it more if you like.

Of course, since I personally don't agree, I'd like to discuss it more
if possible.

> But that is rather different from
> quietly rolling in your choices of behavior change to a "cleanup" patch.

"[PATCH 3/6] signals: use __group_complete_signal() for the specific signals too"
adds a behaviour change too.

Surely, I don't want to change the behaviour quietly, that is why I am
cc'ing maintainers.

(note also these patches are 8/6, 9/6. Originally I was going to send
 them in a separate series).

Oleg.

Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[Roland McGrath]

> Of course, since I personally don't agree, I'd like to discuss it more
> if possible.

Certainly, but let's do it under separate cover, and after some of
these cleanups settle.  (I'd rather not try to get into it this week.)

> "[PATCH 3/6] signals: use __group_complete_signal() for the specific signals too"
> adds a behaviour change too.

Your log entry was explicit about the semantics change there.  You
explained it up front and justified it.  I also happened to agree
with it, but that's a separate issue.

I am certainly not opposed to semantics changes a priori.
I happen to have reservations about this particular one.  

> Surely, I don't want to change the behaviour quietly, that is why I am
> cc'ing maintainers.

The point is that, whenever possible, a semantics change should be
isolated into a patch separate from any related cleanups.  More
important than that, no semantics change should go unmentioned so
it's only documented as a result of someone's careful review of
the change.  (Of course when a change is inadvertent, then only
review is going to notice it--that's what review is for.)

Your new pair of patches dated 2008-3-12 look like they are doing
exactly this (just the cleanup first).  After those are in, the
semantics change you want is a one-liner and easy to review and
discuss on its own.

> (note also these patches are 8/6, 9/6. Originally I was going to send
>  them in a separate series).

I had noticed that your wholes sometimes go up to 1.5; I just
figured it's because you're 50% more thorough than the rest of us.
;-)


Thanks,
Roland

Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

[Oleg Nesterov]

On 03/12, Roland McGrath wrote:
>
> > Of course, since I personally don't agree, I'd like to discuss it more
> > if possible.
> 
> The point is that, whenever possible, a semantics change should be
> isolated into a patch separate from any related cleanups.  More
> important than that, no semantics change should go unmentioned so
> it's only documented as a result of someone's careful review of
> the change.

Yes. You are very right.

And. I must admit. I didn't even realize that the added semantics change
worth at least a special note in changelog because it is easily visible.
This was really wrong in any case. Thanks for pointing out this.

Oleg.