LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@tv-sign.ru>
To: Jiri Kosina <jkosina@suse.cz>
Cc: Roland McGrath <roland@redhat.com>,
	Davide Libenzi <davidel@xmailserver.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [RFC] fix missed SIGCONT cases
Date: Thu, 28 Feb 2008 19:08:34 +0300	[thread overview]
Message-ID: <20080228160834.GA11510@tv-sign.ru> (raw)
In-Reply-To: <Pine.LNX.4.64.0802281633540.3391@jikos.suse.cz>

On 02/28, Jiri Kosina wrote:
>
> On Thu, 28 Feb 2008, Oleg Nesterov wrote:
> 
> > Currently I have the very vagues idea, please see the "patch" below (it 
> > is not right of course, just for illustration).
> > These unlock(->siglock)'s on the signal sending path are really nasty,
> > it would be wonderfull to avoid them. Note also sig_needs_tasklist(),
> > we take tasklist_lock() exactly because we will probably drop siglock.
> 
> Whould it be problematic to change do_notify_parent_cldstop() so that it 
> doesn't acquire siglock, but asumes that is is called with siglock held 
> instead? I can't immediately see any reason why this shouldn't be 
> possible.

Please note that do_notify_parent_cldstop() needs _another_ ->siglock,
the parent's one. We drop target's one because we can't take both, this
is deadlockable.

> > What do you think about the approach at least?
> 
> So it basically does the same thing my patch did -- postpones signalling 
> the parent until it is safe, right?

Well, yes and no. Yes, it postpones signalling, but actually it removes
these do_notify_parent_cldstop()'s from the senfer path completely.
Instead, this task itself (the reciever) should notify the parent.

I am still not sure this doesn't have some fundamental problems, at least
I need to think more... Actually, I hope Roland will shed a light ;)

Hmm... Can't we make a simpler patch for the start? See below.
We can notify the parent earlier, before waking the TASK_STOPPED
child. This should fix this race, no?

Oleg.


--- kernel/signal.c	2008-02-15 16:59:17.000000000 +0300
+++ kernel/signal.c	2008-02-28 19:06:16.970253164 +0300
@@ -600,6 +600,25 @@ static void handle_stop_signal(int sig, 
 			do_notify_parent_cldstop(p, CLD_STOPPED);
 			spin_lock(&p->sighand->siglock);
 		}
+		if (p->signal->flags & SIGNAL_STOP_STOPPED) {
+			/*
+			 * We were in fact stopped, and are now continued.
+			 * Notify the parent with CLD_CONTINUED.
+			 */
+			p->signal->flags = SIGNAL_STOP_CONTINUED;
+			p->signal->group_exit_code = 0;
+			spin_unlock(&p->sighand->siglock);
+			do_notify_parent_cldstop(p, CLD_CONTINUED);
+			spin_lock(&p->sighand->siglock);
+		} else {
+			/*
+			 * We are not stopped, but there could be a stop
+			 * signal in the middle of being processed after
+			 * being removed from the queue.  Clear that too.
+			 */
+			p->signal->flags = 0;
+		}
+
 		rm_from_queue(SIG_KERNEL_STOP_MASK, &p->signal->shared_pending);
 		t = p;
 		do {
@@ -629,25 +648,6 @@ static void handle_stop_signal(int sig, 
 
 			t = next_thread(t);
 		} while (t != p);
-
-		if (p->signal->flags & SIGNAL_STOP_STOPPED) {
-			/*
-			 * We were in fact stopped, and are now continued.
-			 * Notify the parent with CLD_CONTINUED.
-			 */
-			p->signal->flags = SIGNAL_STOP_CONTINUED;
-			p->signal->group_exit_code = 0;
-			spin_unlock(&p->sighand->siglock);
-			do_notify_parent_cldstop(p, CLD_CONTINUED);
-			spin_lock(&p->sighand->siglock);
-		} else {
-			/*
-			 * We are not stopped, but there could be a stop
-			 * signal in the middle of being processed after
-			 * being removed from the queue.  Clear that too.
-			 */
-			p->signal->flags = 0;
-		}
 	} else if (sig == SIGKILL) {
 		/*
 		 * Make sure that any pending stop signal already dequeued


  reply	other threads:[~2008-02-28 16:05 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-27 15:22 Jiri Kosina
2008-02-27 21:00 ` Roland McGrath
2008-02-27 22:04   ` Jiri Kosina
2008-02-28 10:26     ` Roland McGrath
2008-02-28 11:42       ` Jiri Kosina
2008-02-28 15:32         ` Oleg Nesterov
2008-02-28 15:32           ` Jiri Kosina
2008-02-28 15:30       ` Oleg Nesterov
2008-02-28 15:40         ` Jiri Kosina
2008-02-28 16:08           ` Oleg Nesterov [this message]
2008-02-28 16:13             ` Jiri Kosina
2008-02-28 16:45               ` [PATCH] handle_stop_signal: don't wake up the stopped task until it sees SIGCONT Oleg Nesterov
2008-02-28 16:56                 ` Jiri Kosina
2008-02-29  2:39         ` [PATCH] [RFC] fix missed SIGCONT cases Roland McGrath
2008-02-29 12:01           ` Oleg Nesterov
2008-02-29 13:20             ` Oleg Nesterov
2008-03-01  1:59             ` Roland McGrath
2008-03-01 16:41               ` Oleg Nesterov
2008-03-03 13:25                 ` Oleg Nesterov
2008-03-06 10:50                   ` Roland McGrath
2008-03-06  9:05                 ` Roland McGrath

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080228160834.GA11510@tv-sign.ru \
    --to=oleg@tv-sign.ru \
    --cc=akpm@linux-foundation.org \
    --cc=davidel@xmailserver.org \
    --cc=jkosina@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=roland@redhat.com \
    --subject='Re: [PATCH] [RFC] fix missed SIGCONT cases' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).