LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 5/6] signals: unify send_sigqueue/send_group_sigqueue completely
@ 2008-03-08 15:10 Oleg Nesterov
  2008-03-11  2:07 ` Roland McGrath
  0 siblings, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2008-03-08 15:10 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Ingo Molnar, Pavel Emelyanov, Roland McGrath, linux-kernel

Suggested by Pavel Emelyanov.

send_sigqueue/send_group_sigqueue are only differ in how they lock ->siglock.
Unify them. send_group_sigqueue() uses spin_lock() because it knows the task
can't exit, but in that case lock_task_sighand() can't fail and doesn't hurt.

Note that the "sig" argument is ignored, it is always equal to ->si_signo.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 25/kernel/signal.c~5_SSQ_UNIFY	2008-03-08 16:54:57.000000000 +0300
+++ 25/kernel/signal.c	2008-03-08 17:15:59.000000000 +0300
@@ -1240,14 +1240,27 @@ void sigqueue_free(struct sigqueue *q)
 	__sigqueue_free(q);
 }
 
-static int do_send_sigqueue(int sig, struct sigqueue *q, struct task_struct *t,
+static int do_send_sigqueue(struct sigqueue *q, struct task_struct *t,
 				int group)
 {
+	int sig = q->info.si_signo;
 	struct sigpending *pending;
+	unsigned long flags;
+	int ret;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
+
+	ret = -1;
+	if (!likely(lock_task_sighand(t, &flags)))
+		goto ret;
+
 	handle_stop_signal(sig, t);
 
+	ret = 1;
+	if (sig_ignored(t, sig))
+		goto out;
+
+	ret = 0;
 	if (unlikely(!list_empty(&q->list))) {
 		/*
 		 * If an SI_TIMER entry is already queue just increment
@@ -1256,58 +1269,29 @@ static int do_send_sigqueue(int sig, str
 
 		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
-		return 0;
+		goto out;
 	}
 
-	if (sig_ignored(t, sig))
-		return 1;
-
 	signalfd_notify(t, sig);
 	pending = group ? &t->signal->shared_pending : &t->pending;
 	list_add_tail(&q->list, &pending->list);
 	sigaddset(&pending->signal, sig);
 	complete_signal(sig, t, group);
-
-	return 0;
+out:
+	unlock_task_sighand(t, &flags);
+ret:
+	return ret;
 }
 
 int send_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
 {
-	unsigned long flags;
-	int ret = -1;
-
-	/*
-	 * The rcu based delayed sighand destroy makes it possible to
-	 * run this without tasklist lock held. The task struct itself
-	 * cannot go away as create_timer did get_task_struct().
-	 *
-	 * We return -1, when the task is marked exiting, so
-	 * posix_timer_event can redirect it to the group leader
-	 */
-	if (!likely(lock_task_sighand(p, &flags)))
-		goto out_err;
-
-	ret = do_send_sigqueue(sig, q, p, 0);
-
-	unlock_task_sighand(p, &flags);
-out_err:
-	return ret;
+	return do_send_sigqueue(q, p, 0);
 }
 
 int
 send_group_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
 {
-	unsigned long flags;
-	int ret;
-
-	/* Since it_lock is held, p->sighand cannot be NULL. */
-	spin_lock_irqsave(&p->sighand->siglock, flags);
-
-	ret = do_send_sigqueue(sig, q, p, 1);
-
-	spin_unlock_irqrestore(&p->sighand->siglock, flags);
-
-	return ret;
+	return do_send_sigqueue(q, p, 1);
 }
 
 /*


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 5/6] signals: unify send_sigqueue/send_group_sigqueue completely
  2008-03-08 15:10 [PATCH 5/6] signals: unify send_sigqueue/send_group_sigqueue completely Oleg Nesterov
@ 2008-03-11  2:07 ` Roland McGrath
  2008-03-11 18:11   ` Oleg Nesterov
  0 siblings, 1 reply; 4+ messages in thread
From: Roland McGrath @ 2008-03-11  2:07 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: Andrew Morton, Ingo Molnar, Pavel Emelyanov, linux-kernel

Looks fine.  But I'd like a comment on the "ret = 1" case making
explicit what that magic return case is for and why the value
matters (to posix_timer_event).


Thanks,
Roland

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 5/6] signals: unify send_sigqueue/send_group_sigqueue completely
  2008-03-11  2:07 ` Roland McGrath
@ 2008-03-11 18:11   ` Oleg Nesterov
  2008-03-11 20:29     ` Roland McGrath
  0 siblings, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2008-03-11 18:11 UTC (permalink / raw)
  To: Roland McGrath; +Cc: Andrew Morton, Ingo Molnar, Pavel Emelyanov, linux-kernel

On 03/10, Roland McGrath wrote:
>
> Looks fine.  But I'd like a comment on the "ret = 1" case making
> explicit what that magic return case is for and why the value
> matters (to posix_timer_event).

Well, currently this helper is solely used by posix_timer_event(), but despite
the fact it has BUG_ON(!SI_TIMER) it is quite generic, there were patches which
used it for aio.

So I don't think we should document the interaction with posix timers, but OK,
I'll send a small comment update.

Oleg.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 5/6] signals: unify send_sigqueue/send_group_sigqueue completely
  2008-03-11 18:11   ` Oleg Nesterov
@ 2008-03-11 20:29     ` Roland McGrath
  0 siblings, 0 replies; 4+ messages in thread
From: Roland McGrath @ 2008-03-11 20:29 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: Andrew Morton, Ingo Molnar, Pavel Emelyanov, linux-kernel

> So I don't think we should document the interaction with posix timers, but OK,
> I'll send a small comment update.

I didn't say the documentation has to be misleading about the general
utility of the function.  I said it has to explain the subtlety of the
function's calling convention.


Thanks,
Roland

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2008-03-11 20:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-08 15:10 [PATCH 5/6] signals: unify send_sigqueue/send_group_sigqueue completely Oleg Nesterov
2008-03-11  2:07 ` Roland McGrath
2008-03-11 18:11   ` Oleg Nesterov
2008-03-11 20:29     ` Roland McGrath

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).