A couple of very minor nits,
> +static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, siginfo_t *info,
> + int nonblock)
> +{
> + int locked;
> + ssize_t ret;
> + struct signalfd_lockctx lk;
> + DECLARE_WAITQUEUE(wait, current);
> +
> + locked = signalfd_lock(ctx, &lk);
> + if (!locked)
> + return 0;
> +
> + ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
> + switch (ret) {
> + case 0:
> + if (!nonblock)
> + break;
> + ret = -EAGAIN;
> + default:
> + signalfd_unlock(&lk);
> + return ret;
> + }
> +
> + add_wait_queue(&ctx->wqh, &wait);
> + for (;;) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
> + if (ret != 0)
> + break;
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + break;
> + }
> + signalfd_unlock(&lk);
The locking looks a bit overcomplicated. We don't need signalfd_lock() to
check ret != 0 or signal_pending(), we can drop it earlier. This way we
always leave the loop in "unlocked" state.
> + schedule();
> + locked = signalfd_lock(ctx, &lk);
> + if (unlikely(!locked)) {
> + /*
> + * Let the caller read zero byte, ala socket
> + * recv() when the peer disconnect. This test
> + * must be done before doing a dequeue_signal(),
> + * because if the sighand has been orphaned,
> + * the dequeue_signal() call is going to crash.
> + */
Imho, the comment is a bit confusing. dequeue_signal() needs ->siglock
even if signalfd_ctx is not orphaned.
> + ret = 0;
> + break;
> + }
> + }
> +
> + remove_wait_queue(&ctx->wqh, &wait);
> + __set_current_state(TASK_RUNNING);
> +
> + if (likely(locked))
> + signalfd_unlock(&lk);
> +
> + return ret;
> +}
IOW, how about this?
static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, siginfo_t *info,
int nonblock)
{
ssize_t ret;
struct signalfd_lockctx lk;
DECLARE_WAITQUEUE(wait, current);
if (!signalfd_lock(ctx, &lk))
return 0;
ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
switch (ret) {
case 0:
if (!nonblock)
break;
ret = -EAGAIN;
default:
signalfd_unlock(&lk);
return ret;
}
add_wait_queue(&ctx->wqh, &wait);
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
signalfd_unlock(&lk);
if (ret != 0)
break;
ret = -ERESTARTSYS;
if (signal_pending(current))
break;
schedule();
ret = 0;
if (!signalfd_lock(ctx, &lk))
break;
}
remove_wait_queue(&ctx->wqh, &wait);
__set_current_state(TASK_RUNNING);
return ret;
}
Oleg.
On Mon, 21 May 2007, Oleg Nesterov wrote:
> > + schedule();
> > + locked = signalfd_lock(ctx, &lk);
> > + if (unlikely(!locked)) {
> > + /*
> > + * Let the caller read zero byte, ala socket
> > + * recv() when the peer disconnect. This test
> > + * must be done before doing a dequeue_signal(),
> > + * because if the sighand has been orphaned,
> > + * the dequeue_signal() call is going to crash.
> > + */
>
> Imho, the comment is a bit confusing. dequeue_signal() needs ->siglock
> even if signalfd_ctx is not orphaned.
The comment looks clear to me. It states:
1) The policy of returning 0 when the sighand has been detached
2) That we _must_not_ call dequeue_signal() in case signalfd_lock() fails
#ACK on the code mod below.
- Davide
Davide Libenzi wrote:
> On Mon, 21 May 2007, Oleg Nesterov wrote:
>
>>> + schedule();
>>> + locked = signalfd_lock(ctx, &lk);
>>> + if (unlikely(!locked)) {
>>> + /*
>>> + * Let the caller read zero byte, ala socket
>>> + * recv() when the peer disconnect. This test
>>> + * must be done before doing a dequeue_signal(),
>>> + * because if the sighand has been orphaned,
>>> + * the dequeue_signal() call is going to crash.
>>> + */
>> Imho, the comment is a bit confusing. dequeue_signal() needs ->siglock
>> even if signalfd_ctx is not orphaned.
>
> The comment looks clear to me. It states:
>
> 1) The policy of returning 0 when the sighand has been detached
>
> 2) That we _must_not_ call dequeue_signal() in case signalfd_lock() fails
>
> #ACK on the code mod below.
>
> - Davide
>
Andrew, please apply on top.
Simplify signalfd locking following suggestions by Oleg Nesterov.
Signed-off-by: Davi E. M. Arnaut <[email protected]>
Index: linux-2.6/fs/signalfd.c
===================================================================
--- linux-2.6.orig/fs/signalfd.c
+++ linux-2.6/fs/signalfd.c
@@ -211,13 +211,11 @@ static int signalfd_copyinfo(struct sign
static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, siginfo_t *info,
int nonblock)
{
- int locked;
ssize_t ret;
struct signalfd_lockctx lk;
DECLARE_WAITQUEUE(wait, current);
- locked = signalfd_lock(ctx, &lk);
- if (!locked)
+ if (!signalfd_lock(ctx, &lk))
return 0;
ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
@@ -235,24 +233,24 @@ static ssize_t signalfd_dequeue(struct s
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
ret = dequeue_signal(lk.tsk, &ctx->sigmask, info);
+ signalfd_unlock(&lk);
if (ret != 0)
break;
if (signal_pending(current)) {
ret = -ERESTARTSYS;
break;
}
- signalfd_unlock(&lk);
schedule();
- locked = signalfd_lock(ctx, &lk);
- if (unlikely(!locked)) {
+ ret = signalfd_lock(ctx, &lk);
+ if (unlikely(!ret)) {
/*
* Let the caller read zero byte, ala socket
* recv() when the peer disconnect. This test
* must be done before doing a dequeue_signal(),
* because if the sighand has been orphaned,
- * the dequeue_signal() call is going to crash.
+ * the dequeue_signal() call is going to crash
+ * because ->sighand will be long gone.
*/
- ret = 0;
break;
}
}
@@ -260,9 +258,6 @@ static ssize_t signalfd_dequeue(struct s
remove_wait_queue(&ctx->wqh, &wait);
__set_current_state(TASK_RUNNING);
- if (likely(locked))
- signalfd_unlock(&lk);
-
return ret;
}