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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/