Re: [patch 2/5] signalfd v2 - signalfd core ...

[Davide Libenzi <davidel@xmailserver.org>]

On Thu, 8 Mar 2007, Oleg Nesterov wrote:

> Davide Libenzi wrote:
> >
> > +int signalfd_deliver(struct sighand_struct *sighand, int sig, struct siginfo *info)
> > +{
> > +	int nsig = 0;
> > +	struct list_head *pos;
> > +	struct signalfd_ctx *ctx;
> > +	struct signalfd_sq *sq;
> > +
> > +	list_for_each(pos, &sighand->sfdlist) {
> > +		ctx = list_entry(pos, struct signalfd_ctx, lnk);
> > +		/*
> > +		 * We use a negative signal value as a way to broadcast that the
> > +		 * sighand has been orphaned, so that we can notify all the
> > +		 * listeners about this.
> > +		 */
> > +		if (sig < 0)
> > +			__wake_up_locked(&ctx->wqh, TASK_UNINTERRUPTIBLE | TASK_INTERRUPTIBLE);
> > +		else if (sigismember(&ctx->sigmask, sig) &&
> > +			 (sig >= SIGRTMIN || !sigismember(&ctx->pending, sig))) {
> > +			sigaddset(&ctx->pending, sig);
> 
> I don't understand the "(sig >= SIGRTMIN || !sigismember(&ctx->pending, sig))"
> check. This mimics the LEGACY_QUEUE() check, but seems strange. The signal may
> be pending in ctx->pending just because it was not signalfd_fetchsig()ed, yes?

Logic is, if it's not an RT signal, queue only one, otherwise multiple.
The bit on the ->pending mask is clealer only when the queue slot becomes empty.



> Please also see below.
> 
> > +asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemask)
> > +{
> >
> > [...snip...]
> >
> > +	} else {
> > +		error = -EBADF;
> > +		file = fget(ufd);
> > +		if (!file)
> > +			goto err_exit;
> > +		ctx = file->private_data;
> > +		error = -EINVAL;
> > +		if (file->f_op != &signalfd_fops) {
> > +			fput(file);
> > +			goto err_exit;
> > +		}
> > +		spin_lock_irq(&ctx->sighand->siglock);
> > +		ctx->sigmask = sigmask;
> > +		spin_unlock_irq(&ctx->sighand->siglock);
> > +		wake_up(&ctx->wqh);
> 
> Can't this race with sys_signalfd_dequeue() which use lockless __add_wait_queue()?
> Looks like we should do __wake_up_locked() under ctx->sighand->siglock.

Yes, good catch. Fixed.



> > --- linux-2.6.20.ep2.orig/kernel/signal.c	2007-03-07 15:55:43.000000000 -0800
> > +++ linux-2.6.20.ep2/kernel/signal.c	2007-03-07 15:59:01.000000000 -0800
> >
> > [...snip...]
> >
> > @@ -780,6 +785,11 @@
> >  	BUG_ON(!irqs_disabled());
> >  	assert_spin_locked(&t->sighand->siglock);
> >  
> > +	/*
> > +	 * Deliver the signal to listening signalfds ...
> > +	 */
> > +	signalfd_notify(t->sighand, sig, info);
> > +
> >  	/* Short-circuit ignored signals.  */
> >  	if (sig_ignored(t, sig))
> >  		goto out;
> > @@ -968,6 +978,11 @@
> >  	assert_spin_locked(&p->sighand->siglock);
> >  	handle_stop_signal(sig, p);
> >  
> > +	/*
> > +	 * Deliver the signal to listening signalfds ...
> > +	 */
> > +	signalfd_notify(p->sighand, sig, info);
> > +
> >  	/* Short-circuit ignored signals.  */
> >  	if (sig_ignored(p, sig))
> >  		return ret;
> 
> It is strange that we are doing signalfd_notify() even if the signal is ignored.
> Isn't it better to shift signalfd_notify() into send_signal() ? This way we do
> not need the special check in signalfd_deliver() above.

The two trasports can rely on different masks. The signalfd_notify() does 
not even go in signalfd_deliver() if no signalfds are attached to the 
sighand.



> Also, this patch doesn't take send_sigqueue/send_group_sigqueue into account.

I added that too. but I noticed something strange, dunno if intentional or 
not. In send_sigqueue and send_group_sigqueue, the check for the 
timer-special and the ignored is inverted. This lead to two different 
behaviours. Is there a reason for that?




- Davide