Re: [RFC + PATCH] signalfd simplification
On Sun, 2 Sep 2007, Oleg Nesterov wrote: > We can optimize this later, using a "clever" wait_queue_func_t if needed. Good idea. Will do ... - 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/
Re: [RFC + PATCH] signalfd simplification
On 09/02, Oleg Nesterov wrote: > > Small problem: unless I missed something, signalfd_deliver() and > sys_signalfd() > should use wake_up_all(), not wake_up() which implies nr_exclusive == 1. > > It is possible that we have multiple threads waiting on ->signalfd_wqh with > the the different ->sigmask. In this case, the first woken thread can ignore > the signal, we should wake up all of them. Oops, sorry for noise. I forgot about WQ_FLAG_EXCLUSIVE, we don't set it by default. Oleg. - 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/
Re: [RFC + PATCH] signalfd simplification
On 09/01, Davide Libenzi wrote: > > I'm playing at the moment with this patch, that recall Ben's idea of > attaching to the sighand only during read/poll, and calling dequeue_signal() > only with "current". This simplifies the signalfd logic quite a bit. > If this patch is applied, a task calling signalfd can read its own private > signals, and its own group signals. > > fs/exec.c |3 > fs/signalfd.c | 186 > +++--- > include/linux/init_task.h |2 > include/linux/sched.h |2 > include/linux/signalfd.h | 29 --- > kernel/exit.c |9 -- > kernel/fork.c |2 > kernel/signal.c |8 - > 8 files changed, 40 insertions(+), 201 deletions(-) Imho, very very nice. We lose the ability to read the cross-process signals, but I doubt very much we should regret about that. I cc'ed Michael, because it makes sense to document a user-visible change. With this patch, the forked child reads its own signals (not parent's) via the inherited signalfd (or if it was passed with unix socket). Small problem: unless I missed something, signalfd_deliver() and sys_signalfd() should use wake_up_all(), not wake_up() which implies nr_exclusive == 1. It is possible that we have multiple threads waiting on ->signalfd_wqh with the the different ->sigmask. In this case, the first woken thread can ignore the signal, we should wake up all of them. We can optimize this later, using a "clever" wait_queue_func_t if needed. > + spin_lock_irq(>sighand->siglock); > + if (next_signal(>pending, >sigmask) > 0 || > + next_signal(>signal->shared_pending, > + >sigmask) > 0) Very minor nit: next_signal() always returns the value >= 0, imho the "> 0" check looks a bit confusing. Oleg. - 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/
Re: [RFC + PATCH] signalfd simplification
On 09/01, Davide Libenzi wrote: I'm playing at the moment with this patch, that recall Ben's idea of attaching to the sighand only during read/poll, and calling dequeue_signal() only with current. This simplifies the signalfd logic quite a bit. If this patch is applied, a task calling signalfd can read its own private signals, and its own group signals. fs/exec.c |3 fs/signalfd.c | 186 +++--- include/linux/init_task.h |2 include/linux/sched.h |2 include/linux/signalfd.h | 29 --- kernel/exit.c |9 -- kernel/fork.c |2 kernel/signal.c |8 - 8 files changed, 40 insertions(+), 201 deletions(-) Imho, very very nice. We lose the ability to read the cross-process signals, but I doubt very much we should regret about that. I cc'ed Michael, because it makes sense to document a user-visible change. With this patch, the forked child reads its own signals (not parent's) via the inherited signalfd (or if it was passed with unix socket). Small problem: unless I missed something, signalfd_deliver() and sys_signalfd() should use wake_up_all(), not wake_up() which implies nr_exclusive == 1. It is possible that we have multiple threads waiting on -signalfd_wqh with the the different -sigmask. In this case, the first woken thread can ignore the signal, we should wake up all of them. We can optimize this later, using a clever wait_queue_func_t if needed. + spin_lock_irq(current-sighand-siglock); + if (next_signal(current-pending, ctx-sigmask) 0 || + next_signal(current-signal-shared_pending, + ctx-sigmask) 0) Very minor nit: next_signal() always returns the value = 0, imho the 0 check looks a bit confusing. Oleg. - 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/
Re: [RFC + PATCH] signalfd simplification
On Sun, 2 Sep 2007, Oleg Nesterov wrote: We can optimize this later, using a clever wait_queue_func_t if needed. Good idea. Will do ... - 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/
[RFC + PATCH] signalfd simplification
While I was in vacation, I noticed that more "tsk == current" check were
added to the signal logic because of the way signalfd fetches other task
signals.
I'm playing at the moment with this patch, that recall Ben's idea of
attaching to the sighand only during read/poll, and calling dequeue_signal()
only with "current". This simplifies the signalfd logic quite a bit.
If this patch is applied, a task calling signalfd can read its own private
signals, and its own group signals.
Comments?
- Davide
---
fs/exec.c |3
fs/signalfd.c | 186 +++---
include/linux/init_task.h |2
include/linux/sched.h |2
include/linux/signalfd.h | 29 ---
kernel/exit.c |9 --
kernel/fork.c |2
kernel/signal.c |8 -
8 files changed, 40 insertions(+), 201 deletions(-)
Index: linux-2.6.mod/fs/signalfd.c
===
--- linux-2.6.mod.orig/fs/signalfd.c2007-09-01 10:37:39.0 -0700
+++ linux-2.6.mod/fs/signalfd.c 2007-09-01 12:33:37.0 -0700
@@ -11,8 +11,10 @@
* Now using anonymous inode source.
* Thanks to Oleg Nesterov for useful code review and suggestions.
* More comments and suggestions from Arnd Bergmann.
- * Sat May 19, 2007: Davi E. M. Arnaut <[EMAIL PROTECTED]>
+ * Sat May 19, 2007: Davi E. M. Arnaut <[EMAIL PROTECTED]>
* Retrieve multiple signals with one read() call
+ * Sun Jul 15, 2007: Davide Libenzi <[EMAIL PROTECTED]>
+ * Attach to the sighand only during read().
*/
#include
@@ -27,102 +29,20 @@
#include
struct signalfd_ctx {
- struct list_head lnk;
- wait_queue_head_t wqh;
sigset_t sigmask;
- struct task_struct *tsk;
};
-struct signalfd_lockctx {
- struct task_struct *tsk;
- unsigned long flags;
-};
-
-/*
- * Tries to acquire the sighand lock. We do not increment the sighand
- * use count, and we do not even pin the task struct, so we need to
- * do it inside an RCU read lock, and we must be prepared for the
- * ctx->tsk going to NULL (in signalfd_deliver()), and for the sighand
- * being detached. We return 0 if the sighand has been detached, or
- * 1 if we were able to pin the sighand lock.
- */
-static int signalfd_lock(struct signalfd_ctx *ctx, struct signalfd_lockctx *lk)
-{
- struct sighand_struct *sighand = NULL;
-
- rcu_read_lock();
- lk->tsk = rcu_dereference(ctx->tsk);
- if (likely(lk->tsk != NULL))
- sighand = lock_task_sighand(lk->tsk, >flags);
- rcu_read_unlock();
-
- if (!sighand)
- return 0;
-
- if (!ctx->tsk) {
- unlock_task_sighand(lk->tsk, >flags);
- return 0;
- }
-
- if (lk->tsk->tgid == current->tgid)
- lk->tsk = current;
-
- return 1;
-}
-
-static void signalfd_unlock(struct signalfd_lockctx *lk)
-{
- unlock_task_sighand(lk->tsk, >flags);
-}
-
/*
- * This must be called with the sighand lock held.
+ * This must be called with the "tsk" sighand lock held.
*/
void signalfd_deliver(struct task_struct *tsk, int sig)
{
- struct sighand_struct *sighand = tsk->sighand;
- struct signalfd_ctx *ctx, *tmp;
-
- BUG_ON(!sig);
- list_for_each_entry_safe(ctx, tmp, >signalfd_list, 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. Remember the ctx->sigmask is inverted,
-* so if the user is interested in a signal, that corresponding
-* bit will be zero.
-*/
- if (sig < 0) {
- if (ctx->tsk == tsk) {
- ctx->tsk = NULL;
- list_del_init(>lnk);
- wake_up(>wqh);
- }
- } else {
- if (!sigismember(>sigmask, sig))
- wake_up(>wqh);
- }
- }
-}
-
-static void signalfd_cleanup(struct signalfd_ctx *ctx)
-{
- struct signalfd_lockctx lk;
-
- /*
-* This is tricky. If the sighand is gone, we do not need to remove
-* context from the list, the list itself won't be there anymore.
-*/
- if (signalfd_lock(ctx, )) {
- list_del(>lnk);
- signalfd_unlock();
- }
- kfree(ctx);
+ wake_up(>sighand->signalfd_wqh);
}
static int signalfd_release(struct inode *inode, struct file *file)
{
- signalfd_cleanup(file->private_data);
+ kfree(file->private_data);
return 0;
}
@@ -130,23 +50,15 @@
{
struct signalfd_ctx *ctx = file->private_data;
unsigned int events = 0;
- struct signalfd_lockctx lk;
-
