LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@tv-sign.ru>
To: Pavel Emelyanov <xemul@openvz.org>, Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue
Date: Fri, 22 Feb 2008 17:41:55 +0300	[thread overview]
Message-ID: <20080222144155.GA4349@tv-sign.ru> (raw)
In-Reply-To: <47BEC663.7050904@openvz.org>

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.


  reply	other threads:[~2008-02-22 14:38 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-22 12:56 Pavel Emelyanov
2008-02-22 14:41 ` Oleg Nesterov [this message]
2008-02-28  4:54   ` Thomas Gleixner
2008-02-28  6:17     ` Oleg Nesterov
     [not found]       ` <20080228113633.7431C2700FD@magilla.localdomain>
2008-02-28 15:36         ` Oleg Nesterov
2008-02-28 20:14           ` Roland McGrath
2008-02-29 11:32             ` Oleg Nesterov
2008-02-28 22:32         ` Thomas Gleixner
2008-02-28 22:41           ` Roland McGrath
2008-02-29  8:04             ` Thomas Gleixner

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=20080222144155.GA4349@tv-sign.ru \
    --to=oleg@tv-sign.ru \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=xemul@openvz.org \
    --subject='Re: [PATCH 3/3] Consolidate send_sigqueue and send_group_sigqueue' \
    /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).