[PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Pavel Emelyanov]

Both function do the same thing after proper locking, but
with different sigpening structs, so move the common code
into a helper.

After this we have 4 places that look very similar:
send_sigqueue: calls do_send_sigqueue and signal_wakeup
send_group_sigqueue: calls do_send_sigqueue and __group_complete_signal
__group_send_sig_info: calls send_signal and __group_complete_signal
specific_send_sig_info: calls send_signal and signal_wakeup

Besides, send_signal performs actions similar to do_send_sigqueue's
and __group_complete_signal - to signal_wakeup.

It looks like they can be consolidated gracefully.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 kernel/signal.c |   86 ++++++++++++++++++------------------------------------
 1 files changed, 29 insertions(+), 57 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 8b1b404..c6bb821 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1291,10 +1291,33 @@ void sigqueue_free(struct sigqueue *q)
 	__sigqueue_free(q);
 }
 
+static int do_send_sigqueue(int sig, struct sigqueue *q, struct task_struct *t,
+		struct sigpending *pending)
+{
+	if (unlikely(!list_empty(&q->list))) {
+		/*
+		 * If an SI_TIMER entry is already queue just increment
+		 * the overrun count.
+		 */
+
+		BUG_ON(q->info.si_code != SI_TIMER);
+		q->info.si_overrun++;
+		return 0;
+	}
+
+	if (sig_ignored(t, sig))
+		return 1;
+
+	signalfd_notify(t, sig);
+	list_add_tail(&q->list, &pending->list);
+	sigaddset(&pending->signal, sig);
+	return 0;
+}
+
 int send_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
 {
 	unsigned long flags;
-	int ret = 0;
+	int ret = -1;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 
@@ -1308,37 +1331,14 @@ int send_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
 	 */
 	rcu_read_lock();
 
-	if (!likely(lock_task_sighand(p, &flags))) {
-		ret = -1;
+	if (!likely(lock_task_sighand(p, &flags)))
 		goto out_err;
-	}
 
-	if (unlikely(!list_empty(&q->list))) {
-		/*
-		 * If an SI_TIMER entry is already queue just increment
-		 * the overrun count.
-		 */
-		BUG_ON(q->info.si_code != SI_TIMER);
-		q->info.si_overrun++;
-		goto out;
-	}
-	/* Short-circuit ignored signals.  */
-	if (sig_ignored(p, sig)) {
-		ret = 1;
-		goto out;
-	}
-	/*
-	 * Deliver the signal to listening signalfds. This must be called
-	 * with the sighand lock held.
-	 */
-	signalfd_notify(p, sig);
+	ret = do_send_sigqueue(sig, q, p, &p->pending);
 
-	list_add_tail(&q->list, &p->pending.list);
-	sigaddset(&p->pending.signal, sig);
 	if (!sigismember(&p->blocked, sig))
 		signal_wake_up(p, sig == SIGKILL);
 
-out:
 	unlock_task_sighand(p, &flags);
 out_err:
 	rcu_read_unlock();
@@ -1350,7 +1350,7 @@ int
 send_group_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
 {
 	unsigned long flags;
-	int ret = 0;
+	int ret;
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 
@@ -1359,38 +1359,10 @@ send_group_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
 	spin_lock_irqsave(&p->sighand->siglock, flags);
 	handle_stop_signal(sig, p);
 
-	/* Short-circuit ignored signals.  */
-	if (sig_ignored(p, sig)) {
-		ret = 1;
-		goto out;
-	}
-
-	if (unlikely(!list_empty(&q->list))) {
-		/*
-		 * If an SI_TIMER entry is already queue just increment
-		 * the overrun count.  Other uses should not try to
-		 * send the signal multiple times.
-		 */
-		BUG_ON(q->info.si_code != SI_TIMER);
-		q->info.si_overrun++;
-		goto out;
-	} 
-	/*
-	 * Deliver the signal to listening signalfds. This must be called
-	 * with the sighand lock held.
-	 */
-	signalfd_notify(p, sig);
-
-	/*
-	 * Put this signal on the shared-pending queue.
-	 * We always use the shared queue for process-wide signals,
-	 * to avoid several races.
-	 */
-	list_add_tail(&q->list, &p->signal->shared_pending.list);
-	sigaddset(&p->signal->shared_pending.signal, sig);
+	ret = do_send_sigqueue(sig, q, p, &p->signal->shared_pending);
 
 	__group_complete_signal(sig, p);
-out:
+
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
 	read_unlock(&tasklist_lock);
 	return ret;
-- 
1.5.3.4

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Oleg Nesterov]

On 02/22, Pavel Emelyanov wrote:
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1291,10 +1291,33 @@ void sigqueue_free(struct sigqueue *q)
>  	__sigqueue_free(q);
>  }
>  
> +static int do_send_sigqueue(int sig, struct sigqueue *q, struct task_struct *t,
> +		struct sigpending *pending)
> +{
> +	if (unlikely(!list_empty(&q->list))) {
> +		/*
> +		 * If an SI_TIMER entry is already queue just increment
> +		 * the overrun count.
> +		 */
> +
> +		BUG_ON(q->info.si_code != SI_TIMER);
> +		q->info.si_overrun++;
> +		return 0;
> +	}
> +
> +	if (sig_ignored(t, sig))
> +		return 1;
> +
> +	signalfd_notify(t, sig);
> +	list_add_tail(&q->list, &pending->list);
> +	sigaddset(&pending->signal, sig);
> +	return 0;
> +}
> +
>  int send_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
>  {
>  	unsigned long flags;
> -	int ret = 0;
> +	int ret = -1;
>  
>  	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>  
> @@ -1308,37 +1331,14 @@ int send_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
>  	 */
>  	rcu_read_lock();
>  
> -	if (!likely(lock_task_sighand(p, &flags))) {
> -		ret = -1;
> +	if (!likely(lock_task_sighand(p, &flags)))
>  		goto out_err;
> -	}
>  
> -	if (unlikely(!list_empty(&q->list))) {
> -		/*
> -		 * If an SI_TIMER entry is already queue just increment
> -		 * the overrun count.
> -		 */
> -		BUG_ON(q->info.si_code != SI_TIMER);
> -		q->info.si_overrun++;
> -		goto out;
> -	}
> -	/* Short-circuit ignored signals.  */
> -	if (sig_ignored(p, sig)) {
> -		ret = 1;
> -		goto out;
> -	}
> -	/*
> -	 * Deliver the signal to listening signalfds. This must be called
> -	 * with the sighand lock held.
> -	 */
> -	signalfd_notify(p, sig);
> +	ret = do_send_sigqueue(sig, q, p, &p->pending);
>  
> -	list_add_tail(&q->list, &p->pending.list);
> -	sigaddset(&p->pending.signal, sig);
>  	if (!sigismember(&p->blocked, sig))
>  		signal_wake_up(p, sig == SIGKILL);
>  
> -out:
>  	unlock_task_sighand(p, &flags);
>  out_err:
>  	rcu_read_unlock();
> @@ -1350,7 +1350,7 @@ int
>  send_group_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
>  {
>  	unsigned long flags;
> -	int ret = 0;
> +	int ret;
>  
>  	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>  
> @@ -1359,38 +1359,10 @@ send_group_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
>  	spin_lock_irqsave(&p->sighand->siglock, flags);
>  	handle_stop_signal(sig, p);
>  
> -	/* Short-circuit ignored signals.  */
> -	if (sig_ignored(p, sig)) {
> -		ret = 1;
> -		goto out;
> -	}
> -
> -	if (unlikely(!list_empty(&q->list))) {
> -		/*
> -		 * If an SI_TIMER entry is already queue just increment
> -		 * the overrun count.  Other uses should not try to
> -		 * send the signal multiple times.
> -		 */
> -		BUG_ON(q->info.si_code != SI_TIMER);
> -		q->info.si_overrun++;
> -		goto out;
> -	} 
> -	/*
> -	 * Deliver the signal to listening signalfds. This must be called
> -	 * with the sighand lock held.
> -	 */
> -	signalfd_notify(p, sig);
> -
> -	/*
> -	 * Put this signal on the shared-pending queue.
> -	 * We always use the shared queue for process-wide signals,
> -	 * to avoid several races.
> -	 */
> -	list_add_tail(&q->list, &p->signal->shared_pending.list);
> -	sigaddset(&p->signal->shared_pending.signal, sig);
> +	ret = do_send_sigqueue(sig, q, p, &p->signal->shared_pending);
>  
>  	__group_complete_signal(sig, p);
> -out:
> +
>  	spin_unlock_irqrestore(&p->sighand->siglock, flags);
>  	read_unlock(&tasklist_lock);
>  	return ret;
> -- 

Personally, I think this change is very good. But send_sigqueue() and
send_group_sigqueue() have a very subtle difference which I was never
able to understand.

Let's suppose that sigqueue is already queued, and the signal is ignored
(the latter means we should re-schedule cpu timer or handle overrruns).
In that case send_sigqueue() returns 0, but send_group_sigqueue() returns 1.

I think this is not the problem (in fact, I think this patch makes the
behaviour more correct), but I hope Thomas can take a look and confirm.

Oleg.

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Thomas Gleixner]

On Fri, 22 Feb 2008, Oleg Nesterov wrote:
> > -
> > -	if (unlikely(!list_empty(&q->list))) {
> > -		/*
> > -		 * If an SI_TIMER entry is already queue just increment
> > -		 * the overrun count.
> > -		 */
> > -		BUG_ON(q->info.si_code != SI_TIMER);
> > -		q->info.si_overrun++;
> > -		goto out;
> > -	}
> > -	/* Short-circuit ignored signals.  */
> > -	if (sig_ignored(p, sig)) {
> > -		ret = 1;
> > -		goto out;
> > -	}

> >  send_group_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)

> > -	/* Short-circuit ignored signals.  */
> > -	if (sig_ignored(p, sig)) {
> > -		ret = 1;
> > -		goto out;
> > -	}
> > -
> > -	if (unlikely(!list_empty(&q->list))) {
> > -		/*
> > -		 * If an SI_TIMER entry is already queue just increment
> > -		 * the overrun count.  Other uses should not try to
> > -		 * send the signal multiple times.
> > -		 */
> > -		BUG_ON(q->info.si_code != SI_TIMER);
> > -		q->info.si_overrun++;
> > -		goto out;
> > -	} 
> 
> Personally, I think this change is very good. But send_sigqueue() and
> send_group_sigqueue() have a very subtle difference which I was never
> able to understand.
> 
> Let's suppose that sigqueue is already queued, and the signal is ignored
> (the latter means we should re-schedule cpu timer or handle overrruns).
> In that case send_sigqueue() returns 0, but send_group_sigqueue() returns 1.
> 
> I think this is not the problem (in fact, I think this patch makes the
> behaviour more correct), but I hope Thomas can take a look and confirm.

It should not change anything. We should never have a signal enqueued
when it's ignored anyway.

Roland, any insight why this is different aside of a copy and paste
error ?

Thanks,

	tglx

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Oleg Nesterov]

On 02/28, Thomas Gleixner wrote:
>
> On Fri, 22 Feb 2008, Oleg Nesterov wrote:
> > > -
> > > -	if (unlikely(!list_empty(&q->list))) {
> > > -		/*
> > > -		 * If an SI_TIMER entry is already queue just increment
> > > -		 * the overrun count.
> > > -		 */
> > > -		BUG_ON(q->info.si_code != SI_TIMER);
> > > -		q->info.si_overrun++;
> > > -		goto out;
> > > -	}
> > > -	/* Short-circuit ignored signals.  */
> > > -	if (sig_ignored(p, sig)) {
> > > -		ret = 1;
> > > -		goto out;
> > > -	}
> 
> > >  send_group_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
> 
> > > -	/* Short-circuit ignored signals.  */
> > > -	if (sig_ignored(p, sig)) {
> > > -		ret = 1;
> > > -		goto out;
> > > -	}
> > > -
> > > -	if (unlikely(!list_empty(&q->list))) {
> > > -		/*
> > > -		 * If an SI_TIMER entry is already queue just increment
> > > -		 * the overrun count.  Other uses should not try to
> > > -		 * send the signal multiple times.
> > > -		 */
> > > -		BUG_ON(q->info.si_code != SI_TIMER);
> > > -		q->info.si_overrun++;
> > > -		goto out;
> > > -	} 
> > 
> > Personally, I think this change is very good. But send_sigqueue() and
> > send_group_sigqueue() have a very subtle difference which I was never
> > able to understand.
> > 
> > Let's suppose that sigqueue is already queued, and the signal is ignored
> > (the latter means we should re-schedule cpu timer or handle overrruns).
> > In that case send_sigqueue() returns 0, but send_group_sigqueue() returns 1.
> > 
> > I think this is not the problem (in fact, I think this patch makes the
> > behaviour more correct), but I hope Thomas can take a look and confirm.
> 
> It should not change anything. We should never have a signal enqueued
> when it's ignored anyway.

Well, it _is_ possible. Suppose that the signal is both ignored and blocked.
Now, if the task unblocks the signal, it could be ignored, queued, and
sig_ignored() == T.

But yes, I think you are right, this can't change anything. I just wanted
to be sure there is no subtle reason to prefer one way or another.

> Roland, any insight why this is different aside of a copy and paste
> error ?

I guess this was the reason for the difference.

Oleg.

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Oleg Nesterov]

On 02/28, Roland McGrath wrote:
>
> That code dates from the original introduction of those two functions.
> I can't see any reason why those ever had swapped order of the two checks.
> I think it must have just been sloppy coding in the original work.
> 
> I'm not clear on how the already-queued case could ever happen.  Do we
> really need that check at all?  It shouldn't be possible for the timer to
> be firing when it's already queued, because it won't have been reloaded.
> It only reloads via do_schedule_next_timer after it's dequeued, or because
> a 1 return value said it never was queued.

This is true for the posix timers, but posix cpu timers case is different.
Note the run_posix_cpu_timers()->cpu_timer_fire().

Oleg.

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Roland McGrath]

> > I'm not clear on how the already-queued case could ever happen.  Do we
> > really need that check at all?  It shouldn't be possible for the timer to
> > be firing when it's already queued, because it won't have been reloaded.
> > It only reloads via do_schedule_next_timer after it's dequeued, or because
> > a 1 return value said it never was queued.
> 
> This is true for the posix timers, but posix cpu timers case is different.
> Note the run_posix_cpu_timers()->cpu_timer_fire().

Really?  It too reloads the CPU timer only when posix_timer_event returns
nonzero, and otherwise expects do_schedule_next_timer to be called from
signal dequeuing and call posix_cpu_timer_schedule to do the reload.  I
must be missing something (having written the code I am easily deluded into
thinking I know what it's doing).


Thanks,
Roland

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Oleg Nesterov]

On 02/28, Roland McGrath wrote:
>
> > > I'm not clear on how the already-queued case could ever happen.  Do we
> > > really need that check at all?  It shouldn't be possible for the timer to
> > > be firing when it's already queued, because it won't have been reloaded.
> > > It only reloads via do_schedule_next_timer after it's dequeued, or because
> > > a 1 return value said it never was queued.
> > 
> > This is true for the posix timers, but posix cpu timers case is different.
> > Note the run_posix_cpu_timers()->cpu_timer_fire().
> 
> Really?  It too reloads the CPU timer only when posix_timer_event returns
> nonzero, and otherwise expects do_schedule_next_timer to be called from
> signal dequeuing and call posix_cpu_timer_schedule to do the reload.

Ah. Thanks for correcting me! Looks like I forgot this code completely.
Indeed, check_thread/process_timers() removes cpu_timer_list from list.

Oleg.

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Thomas Gleixner]

On Thu, 28 Feb 2008, Roland McGrath wrote:

> That code dates from the original introduction of those two functions.
> I can't see any reason why those ever had swapped order of the two checks.
> I think it must have just been sloppy coding in the original work.
> 
> I'm not clear on how the already-queued case could ever happen.  Do we
> really need that check at all?  It shouldn't be possible for the timer to
> be firing when it's already queued, because it won't have been reloaded.
> It only reloads via do_schedule_next_timer after it's dequeued, or because
> a 1 return value said it never was queued.  Perhaps a timer_settime can
> restart it while queued?  But I don't think it should.
> 
> I think the rules for send_sigqueue and send_group_sigqueue should match.
> Any call that did not result in the signal being queued must return 1.
> If it is in fact possible at all that the signal is already queued, then
> that case must return 0 if the signal will ever be dequeued.
> 
> Looking at this makes me think we are incorrect in another anal case.
> Say the timer signal is blocked and the timer has fired, so it is on the
> queue and expects to be reloaded by do_schedule_next_timer at dequeue.
> Now, sigaction is used to set that signal to SIG_IGN, so the pending
> signal is removed from the queue but not dequeued.  Time passes, so the
> timer has in theory fired and reloaded several times counted as overruns.
> Next, sigaction is used to establish a handler for that signal, and then
> we unblock it too.  Time continues to pass, but the timer has not been
> reloaded after the first firing, and so it never fires with the handled
> signal though it now should have done so.  This case is at least a bit
> pathological.  But I'm not sure there is anything in POSIX to exempt us
> from making it work correctly.  Off hand the change that seems like it
> would work is to do the dequeue-time SI_TIMER check at discard time too.

Right. I was working on a fix for that for a different reason. What we
really want is a check for such a timer on a transition from SIGIGN to
another state.  This would allow us to drop the ugly auto rearm code
for ignored timers completely. I had some working proof of concept
code, but got distracted. Need to find it again and dust if off.

Thanks,

	tglx

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Roland McGrath]

> Right. I was working on a fix for that for a different reason. What we
> really want is a check for such a timer on a transition from SIGIGN to
> another state.  This would allow us to drop the ugly auto rearm code
> for ignored timers completely. I had some working proof of concept
> code, but got distracted. Need to find it again and dust if off.

That is certainly what came first to mind.  (Note that it's not strictly
SIG_IGN alone, but sigaction_says_ignore.)  We'll have to contemplate a lot
of corners to be sure we get it right.  It's not real clear to me how you
go about finding all the affected timers sanely from sigaction.


Thanks,
Roland

Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue

[Thomas Gleixner]

On Thu, 28 Feb 2008, Roland McGrath wrote:

> > Right. I was working on a fix for that for a different reason. What we
> > really want is a check for such a timer on a transition from SIGIGN to
> > another state.  This would allow us to drop the ugly auto rearm code
> > for ignored timers completely. I had some working proof of concept
> > code, but got distracted. Need to find it again and dust if off.
> 
> That is certainly what came first to mind.  (Note that it's not strictly
> SIG_IGN alone, but sigaction_says_ignore.)  We'll have to contemplate a lot
> of corners to be sure we get it right.  It's not real clear to me how you
> go about finding all the affected timers sanely from sigaction.

My idea was to dequeue the timer signal and put it into some separate
"check me on unignore" queue. That's the easy part. The nasty part is
to rearm the timer without running into locking madness.

Thanks,

	tglx