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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
