waitfd

waitfd provides a file descriptor out of which may be read a series of siginfo_t objects describing child death. A child process is reaped as soon as its information is read. This means child monitoring too can be performed with that same poll call.

Patch is against v2.6.28

Linux now exposes signals, timers, and events via file descriptors through signalfd, timerfd, and eventfd. This means programmers can use a single select/[e]poll call to monitor all change in their program. This patch aims to expose child death via the same mechanism.

waitfd provides a file descriptor out of which may be read a series of siginfo_t objects describing child death. A child process is reaped as soon as its information is read. This means child monitoring too can be performed with that same poll call.

Patch is against v2.6.28

--CJD

arch/x86/include/asm/unistd_32.h +1 -0

index f2bba78..134d83c 100644
@@ -338,6 +338,7 @@
 #define __NR_dup3        330
 #define __NR_pipe2        331
 #define __NR_inotify_init1    332
+#define __NR_waitfd        333
 
 #ifdef __KERNEL__
 

arch/x86/include/asm/unistd_64.h +0 -0

index d2e415e..b28eb07 100644

arch/x86/include/asm/unistd_64.h +0 -0

 #ifndef __NO_STUBS

arch/x86/kernel/syscall_table_32.S +1 -0

index d44395f..c796a8b 100644
@@ -332,3 +332,4 @@ ENTRY(sys_call_table)
    .long sys_dup3            /* 330 */
    .long sys_pipe2
    .long sys_inotify_init1
+    .long sys_waitfd

fs/Makefile +0 -0

index d9f8afe..74c31fb 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_INOTIFY_USER)    += inotify_user.o

obj-$(CONFIG_EPOLL) += eventpoll.o
obj-$(CONFIG_ANON_INODES) += anon_inodes.o
obj-$(CONFIG_SIGNALFD) += signalfd.o

+obj-$(CONFIG_WAITFD)        += waitfd.o
obj-$(CONFIG_TIMERFD)        += timerfd.o
obj-$(CONFIG_EVENTFD)        += eventfd.o

obj-$(CONFIG_AIO) += aio.o

fs/waitfd.c new file +19 -0

new file mode 100644
index 0000000..0155a83
@@ -0,0 +1,117 @@
+/*
+ *  fs/waitfd.c
+ *
+ *  Copyright (C) 2008  Red Hat, Casey Dahlin <[email protected]>
+ *
+ *  Largely derived from fs/signalfd.c
+ */
+
+#include <linux/file.h>
+#include <linux/poll.h>
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/sched.h>
+#include <linux/kernel.h>
+#include <linux/signal.h>
+#include <linux/list.h>
+#include <linux/anon_inodes.h>
+#include <linux/syscalls.h>
+

fs/waitfd.c new file +24 -0

+static int waitfd_release(struct inode *inode, struct file *file)
+{
+    kfree(file->private_data);
+    return 0;
+}
+
+static unsigned int waitfd_poll(struct file *file, poll_table *wait)
+{
+    struct waitfd_ctx *ctx = file->private_data;
+    long value;
+
+    poll_wait(file, &current->signal->wait_chldexit, wait);
+
+    value = do_waitid(ctx->which, ctx->upid, NULL,
+               ctx->ops | WNOHANG | WNOWAIT, NULL);
+    if (value > 0 || value == -ECHILD)
+        return POLLIN;
+
+    return 0;
+}
+
+/*
+ * Returns a multiple of the size of a struct siginfo, or a negative
+ * error code. The "count" parameter must be at least sizeof(struct

siginfo)
+ */
+static ssize_t waitfd_read(struct file *file, char __user *buf, size_t count,

fs/waitfd.c new file +16 -0

+                 loff_t *ppos)
+{
+    struct waitfd_ctx *ctx = file->private_data;
+    struct siginfo __user *info_addr = (struct siginfo *)buf;
+    int flags = ctx->ops;
+    ssize_t ret, total = 0;
+
+    count /= sizeof(struct siginfo);
+    if (!count)
+        return -EINVAL;
+
+    do {
+        ret = do_waitid(ctx->which, ctx->upid, info_addr, flags, NULL);
+        if (ret == 0)
+            ret = -EAGAIN;
+        if (ret == -ECHILD)

fs/waitfd.c new file +10 -0

+}
+
+static const struct file_operations waitfd_fops = {
+    .release    = waitfd_release,
+    .poll        = waitfd_poll,
+    .read        = waitfd_read,
+};
+
+asmlinkage long sys_waitfd(int which, pid_t upid, int options, int unused)
+{

fs/waitfd.c new file +16 -0

+
+    ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+    if (!ctx)
+        return -ENOMEM;
+
+    ctx->ops = options;
+    ctx->upid = upid;
+    ctx->which = which;
+
+    ufd = anon_inode_getfd("[waitfd]", &waitfd_fops, ctx,
+                   (options & WNOHANG) ? O_NONBLOCK : 0);
+    if (ufd < 0)
+        kfree(ctx);
+
+    return ufd;
+}

init/Kconfig +0 -0

index f763762..bc34871 100644

init/Kconfig +4 -0

+      changes from a file descriptor.
+
+      If unsure, say Y.
+

config SIGNALFD
bool "Enable signalfd() system call" if EMBEDDED select ANON_INODES

kernel/exit.c +14 -12

index 2d8be7e..b53e8ba 100644
@@ -1233,18 +1233,20 @@ static int wait_noreap_copyout(struct
 task_struct *p, pid_t pid, uid_t uid,
    int retval = rusagep ? getrusage(p, RUSAGE_BOTH, rusagep) : 0;
 
    put_task_struct(p);
-    if (!retval)
-        retval = put_user(SIGCHLD, &infop->si_signo);
-    if (!retval)
-        retval = put_user(0, &infop->si_errno);
-    if (!retval)
-        retval = put_user((short)why, &infop->si_code);
-    if (!retval)
-        retval = put_user(pid, &infop->si_pid);
-    if (!retval)
-        retval = put_user(uid, &infop->si_uid);
-    if (!retval)
-        retval = put_user(status, &infop->si_status);
+    if (infop) {
+        if (!retval)
+            retval = put_user(SIGCHLD, &infop->si_signo);
+        if (!retval)
+            retval = put_user(0, &infop->si_errno);
+        if (!retval)
+            retval = put_user((short)why, &infop->si_code);
+        if (!retval)
+            retval = put_user(pid, &infop->si_pid);
+        if (!retval)
+            retval = put_user(uid, &infop->si_uid);
+        if (!retval)
+            retval = put_user(status, &infop->si_status);
+    }
    if (!retval)
        retval = pid;

    return retval;

kernel/exit.c +0 -0

@@ -1727,35 +1729,12 @@ repeat:

end:

    current->state = TASK_RUNNING;
    remove_wait_queue(&current->signal->wait_chldexit,&wait);

kernel/exit.c +0 -23

-    if (infop) {
-        if (retval > 0)
-            retval = 0;
-        else {
-            /*
-             * For a WNOHANG return, clear out all the fields
-             * we would set so the user can easily tell the
-             * difference.
-             */
-            if (!retval)
-                retval = put_user(0, &infop->si_signo);
-            if (!retval)
-                retval = put_user(0, &infop->si_errno);
-            if (!retval)
-                retval = put_user(0, &infop->si_code);
-            if (!retval)
-                retval = put_user(0, &infop->si_pid);
-            if (!retval)
-                retval = put_user(0, &infop->si_uid);
-            if (!retval)
-                retval = put_user(0, &infop->si_status);
-        }
-    }
    return retval;

}

kernel/exit.c +3 -3

-asmlinkage long sys_waitid(int which, pid_t upid,
-               struct siginfo __user *infop, int options,
-               struct rusage __user *ru)
+long do_waitid(int which, pid_t upid,
+           struct siginfo __user *infop, int options,
+           struct rusage __user *ru)

{
    struct pid *pid = NULL;
    enum pid_type type;

kernel/exit.c +33 -0

@@ -1789,6 +1768,39 @@ asmlinkage long sys_waitid(int which, pid_t upid,
    ret = do_wait(type, pid, options, infop, NULL, ru);
    put_pid(pid);
 
+    return ret;
+}
+
+asmlinkage long sys_waitid(int which, pid_t upid,
+               struct siginfo __user *infop, int options,
+               struct rusage __user *ru)
+{
+    long ret;
+
+    ret = do_waitid(which, upid, infop, options, ru);
+
+    if (ret > 0)
+        ret = 0;
+    else {
+        /*
+         * For a WNOHANG return, clear out all the fields
+         * we would set so the user can easily tell the
+         * difference.
+         */
+        if (!ret)
+            ret = put_user(0, &infop->si_signo);
+        if (!ret)
+            ret = put_user(0, &infop->si_errno);
+        if (!ret)
+            ret = put_user(0, &infop->si_code);
+        if (!ret)
+            ret = put_user(0, &infop->si_pid);
+        if (!ret)
+            ret = put_user(0, &infop->si_uid);
+        if (!ret)
+            ret = put_user(0, &infop->si_status);
+    }
+
    /* avoid REGPARM breakage on x86: */
    asmlinkage_protect(5, ret, which, upid, infop, options, ru);
    return ret;

kernel/sys_ni.c +1 -0

index e14a232..e8d4da6 100644
@@ -163,6 +163,7 @@ cond_syscall(sys_ioprio_set);
 cond_syscall(sys_ioprio_get);
 
 /* New file descriptors */
+cond_syscall(sys_waitfd);
 cond_syscall(sys_signalfd);
 cond_syscall(sys_signalfd4);
 cond_syscall(compat_sys_signalfd);

Quoted 27 lines · Casey Dahlin

Linux now exposes signals, timers, and events via file descriptors through signalfd, timerfd, and eventfd. This means programmers can use a single select/[e]poll call to monitor all change in their program. This patch aims to expose child death via the same mechanism.

waitfd provides a file descriptor out of which may be read a series of siginfo_t objects describing child death. A child process is reaped as soon as its information is read. This means child monitoring too can be performed with that same poll call.

Patch is against v2.6.28

--CJD

arch/x86/include/asm/unistd_32.h +1 -0

index f2bba78..134d83c 100644
@@ -338,6 +338,7 @@
 #define __NR_dup3        330
 #define __NR_pipe2        331
 #define __NR_inotify_init1    332
+#define __NR_waitfd        333
 
 #ifdef __KERNEL__
 

arch/x86/include/asm/unistd_64.h __SYSCALL(__NR_dup3, sys_dup3) · line 659

Quoted 13 lines

arch/x86/include/asm/unistd_64.h +2 -0

index d2e415e..b28eb07 100644
@@ -653,6 +653,8 @@ __SYSCALL(__NR_dup3, sys_dup3)
 __SYSCALL(__NR_pipe2, sys_pipe2)
 #define __NR_inotify_init1            294
 __SYSCALL(__NR_inotify_init1, sys_inotify_init1)
+#define __NR_waitfd                295
+__SYSCALL(__NR_waitfd, sys_waitfd)
 
 

Only for x86??

Quoted 54 lines

arch/x86/include/asm/unistd_64.h +0 -0

 #ifndef __NO_STUBS

arch/x86/kernel/syscall_table_32.S +1 -0

index d44395f..c796a8b 100644
@@ -332,3 +332,4 @@ ENTRY(sys_call_table)
    .long sys_dup3            /* 330 */
    .long sys_pipe2
    .long sys_inotify_init1
+    .long sys_waitfd

fs/Makefile +0 -0

index d9f8afe..74c31fb 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_INOTIFY_USER)    += inotify_user.o

obj-$(CONFIG_EPOLL) += eventpoll.o
obj-$(CONFIG_ANON_INODES) += anon_inodes.o
obj-$(CONFIG_SIGNALFD) += signalfd.o

+obj-$(CONFIG_WAITFD)        += waitfd.o
obj-$(CONFIG_TIMERFD)        += timerfd.o
obj-$(CONFIG_EVENTFD)        += eventfd.o

obj-$(CONFIG_AIO) += aio.o

fs/waitfd.c new file +25 -0

new file mode 100644
index 0000000..0155a83
@@ -0,0 +1,117 @@
+/*
+ *  fs/waitfd.c
+ *
+ *  Copyright (C) 2008  Red Hat, Casey Dahlin <[email protected]>
+ *
+ *  Largely derived from fs/signalfd.c
+ */
+
+#include <linux/file.h>
+#include <linux/poll.h>
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/sched.h>
+#include <linux/kernel.h>
+#include <linux/signal.h>
+#include <linux/list.h>
+#include <linux/anon_inodes.h>
+#include <linux/syscalls.h>
+
+long do_waitid(int which, pid_t upid,
+           struct siginfo __user *infop, int options,
+           struct rusage __user *ru);
+
+struct waitfd_ctx {
+    int ops;

fs/waitfd.c line 29

Quoted 5 lines

fs/waitfd.c new file +4 -0

+    int which;
+    pid_t upid;
+};
+

Please use kernel coding style: use tabs to indent, not <lots-of-spaces>, and struct members, functions, etc., are indented by one tab stop minimum.

Quoted 45 lines

fs/waitfd.c new file +24 -0

+static int waitfd_release(struct inode *inode, struct file *file)
+{
+    kfree(file->private_data);
+    return 0;
+}
+
+static unsigned int waitfd_poll(struct file *file, poll_table *wait)
+{
+    struct waitfd_ctx *ctx = file->private_data;
+    long value;
+
+    poll_wait(file, &current->signal->wait_chldexit, wait);
+
+    value = do_waitid(ctx->which, ctx->upid, NULL,
+               ctx->ops | WNOHANG | WNOWAIT, NULL);
+    if (value > 0 || value == -ECHILD)
+        return POLLIN;
+
+    return 0;
+}
+
+/*
+ * Returns a multiple of the size of a struct siginfo, or a negative
+ * error code. The "count" parameter must be at least sizeof(struct

siginfo)
+ */
+static ssize_t waitfd_read(struct file *file, char __user *buf, size_t count,

fs/waitfd.c new file +16 -0

+                 loff_t *ppos)
+{
+    struct waitfd_ctx *ctx = file->private_data;
+    struct siginfo __user *info_addr = (struct siginfo *)buf;
+    int flags = ctx->ops;
+    ssize_t ret, total = 0;
+
+    count /= sizeof(struct siginfo);
+    if (!count)
+        return -EINVAL;
+
+    do {
+        ret = do_waitid(ctx->which, ctx->upid, info_addr, flags, NULL);
+        if (ret == 0)
+            ret = -EAGAIN;
+        if (ret == -ECHILD)

fs/waitfd.c line 81

Quoted 10 lines

fs/waitfd.c new file +10 -0

+            ret = 0;
+        if (ret <= 0)
+            break;
+
+        info_addr++;
+        total += sizeof(struct siginfo);
+        flags |= WNOHANG;
+    } while (--count);
+
+    return total ? total: ret;

	return total ? total : ret;

please.

Quoted 15 lines

fs/waitfd.c new file +15 -0

+}
+
+static const struct file_operations waitfd_fops = {
+    .release    = waitfd_release,
+    .poll        = waitfd_poll,
+    .read        = waitfd_read,
+};
+
+asmlinkage long sys_waitfd(int which, pid_t upid, int options, int unused)
+{
+    int ufd;
+    struct waitfd_ctx *ctx;
+
+    /* Just to make sure we don't end up with a sys_waitfd4 */
+    (void)unused;

fs/waitfd.c line 101

Quoted 6 lines

fs/waitfd.c new file +5 -0

+
+    if (options & ~(WNOHANG|WEXITED|WSTOPPED|WCONTINUED))
+        return -EINVAL;
+    if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
+        return -EINVAL;

Use spaces around '|'.

Quoted 21 lines

fs/waitfd.c new file +16 -0

+
+    ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+    if (!ctx)
+        return -ENOMEM;
+
+    ctx->ops = options;
+    ctx->upid = upid;
+    ctx->which = which;
+
+    ufd = anon_inode_getfd("[waitfd]", &waitfd_fops, ctx,
+                   (options & WNOHANG) ? O_NONBLOCK : 0);
+    if (ufd < 0)
+        kfree(ctx);
+
+    return ufd;
+}

init/Kconfig +0 -0

index f763762..bc34871 100644

init/Kconfig config EPOLL · line 692

Quoted 10 lines

init/Kconfig +6 -0

@@ -683,6 +683,16 @@ config EPOLL
      Disabling this option will cause the kernel to be built without
      support for epoll family of system calls.
 
+config WAITFD
+    bool "Enable waitfd() system call" if EMBEDDED
+    select ANON_INODES
+    default y
+    help
+      Enable the waitfd() system call that allows receving child state
 

receiving

Kconfig help text should be indented by <tab><space><space>.

Quoted 131 lines

init/Kconfig +4 -0

+      changes from a file descriptor.
+
+      If unsure, say Y.
+

config SIGNALFD
bool "Enable signalfd() system call" if EMBEDDED select ANON_INODES

kernel/exit.c +14 -12

index 2d8be7e..b53e8ba 100644
@@ -1233,18 +1233,20 @@ static int wait_noreap_copyout(struct
 task_struct *p, pid_t pid, uid_t uid,
    int retval = rusagep ? getrusage(p, RUSAGE_BOTH, rusagep) : 0;
 
    put_task_struct(p);
-    if (!retval)
-        retval = put_user(SIGCHLD, &infop->si_signo);
-    if (!retval)
-        retval = put_user(0, &infop->si_errno);
-    if (!retval)
-        retval = put_user((short)why, &infop->si_code);
-    if (!retval)
-        retval = put_user(pid, &infop->si_pid);
-    if (!retval)
-        retval = put_user(uid, &infop->si_uid);
-    if (!retval)
-        retval = put_user(status, &infop->si_status);
+    if (infop) {
+        if (!retval)
+            retval = put_user(SIGCHLD, &infop->si_signo);
+        if (!retval)
+            retval = put_user(0, &infop->si_errno);
+        if (!retval)
+            retval = put_user((short)why, &infop->si_code);
+        if (!retval)
+            retval = put_user(pid, &infop->si_pid);
+        if (!retval)
+            retval = put_user(uid, &infop->si_uid);
+        if (!retval)
+            retval = put_user(status, &infop->si_status);
+    }
    if (!retval)
        retval = pid;

    return retval;

kernel/exit.c +0 -0

@@ -1727,35 +1729,12 @@ repeat:

end:

    current->state = TASK_RUNNING;
    remove_wait_queue(&current->signal->wait_chldexit,&wait);

kernel/exit.c +0 -23

-    if (infop) {
-        if (retval > 0)
-            retval = 0;
-        else {
-            /*
-             * For a WNOHANG return, clear out all the fields
-             * we would set so the user can easily tell the
-             * difference.
-             */
-            if (!retval)
-                retval = put_user(0, &infop->si_signo);
-            if (!retval)
-                retval = put_user(0, &infop->si_errno);
-            if (!retval)
-                retval = put_user(0, &infop->si_code);
-            if (!retval)
-                retval = put_user(0, &infop->si_pid);
-            if (!retval)
-                retval = put_user(0, &infop->si_uid);
-            if (!retval)
-                retval = put_user(0, &infop->si_status);
-        }
-    }
    return retval;

}

kernel/exit.c +3 -3

-asmlinkage long sys_waitid(int which, pid_t upid,
-               struct siginfo __user *infop, int options,
-               struct rusage __user *ru)
+long do_waitid(int which, pid_t upid,
+           struct siginfo __user *infop, int options,
+           struct rusage __user *ru)

{
    struct pid *pid = NULL;
    enum pid_type type;

kernel/exit.c +33 -0

@@ -1789,6 +1768,39 @@ asmlinkage long sys_waitid(int which, pid_t upid,
    ret = do_wait(type, pid, options, infop, NULL, ru);
    put_pid(pid);
 
+    return ret;
+}
+
+asmlinkage long sys_waitid(int which, pid_t upid,
+               struct siginfo __user *infop, int options,
+               struct rusage __user *ru)
+{
+    long ret;
+
+    ret = do_waitid(which, upid, infop, options, ru);
+
+    if (ret > 0)
+        ret = 0;
+    else {
+        /*
+         * For a WNOHANG return, clear out all the fields
+         * we would set so the user can easily tell the
+         * difference.
+         */
+        if (!ret)
+            ret = put_user(0, &infop->si_signo);
+        if (!ret)
+            ret = put_user(0, &infop->si_errno);
+        if (!ret)
+            ret = put_user(0, &infop->si_code);
+        if (!ret)
+            ret = put_user(0, &infop->si_pid);
+        if (!ret)
+            ret = put_user(0, &infop->si_uid);
+        if (!ret)
+            ret = put_user(0, &infop->si_status);
+    }
+
    /* avoid REGPARM breakage on x86: */
    asmlinkage_protect(5, ret, which, upid, infop, options, ru);
    return ret;

kernel/sys_ni.c +0 -0

index e14a232..e8d4da6 100644
@@ -163,6 +163,7 @@ cond_syscall(sys_ioprio_set);
 cond_syscall(sys_ioprio_get);
 

kernel/sys_ni.c cond_syscall(sys_ioprio_set); · line 169

Quoted 6 lines

kernel/sys_ni.c +1 -0

 /* New file descriptors */
+cond_syscall(sys_waitfd);
 cond_syscall(sys_signalfd);
 cond_syscall(sys_signalfd4);
 cond_syscall(compat_sys_signalfd);

Quoted 29 lines · Randy Dunlap

Quoted 27 lines · Casey Dahlin

Linux now exposes signals, timers, and events via file descriptors through signalfd, timerfd, and eventfd. This means programmers can use a single select/[e]poll call to monitor all change in their program. This patch aims to expose child death via the same mechanism.

waitfd provides a file descriptor out of which may be read a series of siginfo_t objects describing child death. A child process is reaped as soon as its information is read. This means child monitoring too can be performed with that same poll call.

Patch is against v2.6.28

--CJD

arch/x86/include/asm/unistd_32.h +1 -0

index f2bba78..134d83c 100644
@@ -338,6 +338,7 @@
 #define __NR_dup3        330
 #define __NR_pipe2        331
 #define __NR_inotify_init1    332
+#define __NR_waitfd        333
 
 #ifdef __KERNEL__
 

Quoted 17 lines

arch/x86/include/asm/unistd_64.h __SYSCALL(__NR_dup3, sys_dup3) · line 659

Quoted 13 lines

arch/x86/include/asm/unistd_64.h +2 -0

index d2e415e..b28eb07 100644
@@ -653,6 +653,8 @@ __SYSCALL(__NR_dup3, sys_dup3)
 __SYSCALL(__NR_pipe2, sys_pipe2)
 #define __NR_inotify_init1            294
 __SYSCALL(__NR_inotify_init1, sys_inotify_init1)
+#define __NR_waitfd                295
+__SYSCALL(__NR_waitfd, sys_waitfd)
 
 

Only for x86??

At the moment. I should have mentioned this earlier but I haven't made the syscall table entries for archs I don't test on. That will change once the rest of the change has settled out.

Quoted 33 lines

Quoted 33 lines

fs/waitfd.c new file +27 -0

new file mode 100644
index 0000000..0155a83
@@ -0,0 +1,117 @@
+/*
+ *  fs/waitfd.c
+ *
+ *  Copyright (C) 2008  Red Hat, Casey Dahlin <[email protected]>
+ *
+ *  Largely derived from fs/signalfd.c
+ */
+
+#include <linux/file.h>
+#include <linux/poll.h>
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/sched.h>
+#include <linux/kernel.h>
+#include <linux/signal.h>
+#include <linux/list.h>
+#include <linux/anon_inodes.h>
+#include <linux/syscalls.h>
+
+long do_waitid(int which, pid_t upid,
+           struct siginfo __user *infop, int options,
+           struct rusage __user *ru);
+
+struct waitfd_ctx {
+    int ops;
+    int which;
+    pid_t upid;

Quoted 10 lines

fs/waitfd.c line 29

Quoted 3 lines

fs/waitfd.c new file +2 -0

+};
+

Please use kernel coding style: use tabs to indent, not <lots-of-spaces>,
and struct members, functions, etc., are indented by one tab stop minimum.

Damnit. This is a mailer artifact. This is the first time thunderbird has eaten a patch on me. I'll look in to it.

Quoted 16 lines

Quoted 16 lines

fs/waitfd.c new file +9 -0

+}
+
+static const struct file_operations waitfd_fops = {
+    .release    = waitfd_release,
+    .poll        = waitfd_poll,
+    .read        = waitfd_read,
+};
+
+asmlinkage long sys_waitfd(int which, pid_t upid, int options, int

unused)

fs/waitfd.c new file +6 -0

+{
+    int ufd;
+    struct waitfd_ctx *ctx;
+
+    /* Just to make sure we don't end up with a sys_waitfd4 */
+    (void)unused;

Quoted 10 lines

fs/waitfd.c line 101

Quoted 6 lines

fs/waitfd.c new file +5 -0

+
+    if (options & ~(WNOHANG|WEXITED|WSTOPPED|WCONTINUED))
+        return -EINVAL;
+    if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
+        return -EINVAL;

Use spaces around '|'.

Those 4 lines are copied almost exactly from kernel/exit.c. Is there motivation to keep them consistent?

Linux now exposes signals, timers, and events via file descriptors through signalfd, timerfd, and eventfd. This means programmers can use a single select/[e]poll call to monitor all change in their program. This patch aims to expose child death via the same mechanism.

waitfd provides a file descriptor out of which may be read a series of siginfo_t objects describing child death. A child process is reaped as soon as its information is read. This means child monitoring too can be performed with that same poll call.

Patch is against v2.6.28

Original description:

Linux now exposes signals, timers, and events via file descriptors through signalfd, timerfd, and eventfd. This means programmers can use a single select/[e]poll call to monitor all change in their program. This patch aims to expose child death via the same mechanism.

waitfd provides a file descriptor out of which may be read a series of siginfo_t objects describing child death. A child process is reaped as soon as its information is read. This means child monitoring too can be performed with that same poll call.

Patch is against v2.6.28

fs/waitfd.c new file +42 -0

+static int waitfd_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
+static unsigned int waitfd_poll(struct file *file, poll_table *wait)
+{
+	struct waitfd_ctx *ctx = file->private_data;
+	long value;
+
+	poll_wait(file, &current->signal->wait_chldexit, wait);
+
+	value = do_waitid(ctx->which, ctx->upid, NULL,
+			   ctx->ops | WNOHANG | WNOWAIT, NULL);
+	if (value > 0 || value == -ECHILD)
+		return POLLIN;
+
+	return 0;
+}
+
+/*
+ * Returns a multiple of the size of a struct siginfo, or a negative
+ * error code. The "count" parameter must be at least sizeof(struct siginfo)
+ */
+static ssize_t waitfd_read(struct file *file, char __user *buf, size_t count,
+			     loff_t *ppos)
+{
+	struct waitfd_ctx *ctx = file->private_data;
+	struct siginfo __user *info_addr = (struct siginfo *)buf;
+	int flags = ctx->ops;
+	ssize_t ret, total = 0;
+
+	count /= sizeof(struct siginfo);
+	if (!count)
+		return -EINVAL;
+
+	do {
+		ret = do_waitid(ctx->which, ctx->upid, info_addr, flags, NULL);
+		if (ret == 0)
+			ret = -EAGAIN;
+		if (ret == -ECHILD)

fs/waitfd.c new file +6 -0

+	return total ? total : ret;
 
+}
+
+static const struct file_operations waitfd_fops = {
+	.release	= waitfd_release,
+	.poll		= waitfd_poll,

fs/waitfd.c new file +21 -0

+
+	if (options & ~(WNOHANG|WEXITED|WSTOPPED|WCONTINUED))
+		return -EINVAL;
+	if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
+		return -EINVAL;
+
+	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->ops = options;
+	ctx->upid = upid;
+	ctx->which = which;
+
+	ufd = anon_inode_getfd("[waitfd]", &waitfd_fops, ctx,
+			       (options & WNOHANG) ? O_NONBLOCK : 0);
+	if (ufd < 0)
+		kfree(ctx);
+
+	return ufd;
+}

init/Kconfig +0 -0

index f763762..bc34871 100644

init/Kconfig +4 -0

+	  changes from a file descriptor.
+
+	  If unsure, say Y.
+
 config SIGNALFD
 	bool "Enable signalfd() system call" if EMBEDDED
 	select ANON_INODES

kernel/exit.c +38 -38

index 2d8be7e..b53e8ba 100644
@@ -1233,18 +1233,20 @@ static int wait_noreap_copyout(struct task_struct *p, pid_t pid, uid_t uid,
 	int retval = rusagep ? getrusage(p, RUSAGE_BOTH, rusagep) : 0;
 
 	put_task_struct(p);
-	if (!retval)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval)
-		retval = put_user((short)why, &infop->si_code);
-	if (!retval)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval)
-		retval = put_user(uid, &infop->si_uid);
-	if (!retval)
-		retval = put_user(status, &infop->si_status);
+	if (infop) {
+		if (!retval)
+			retval = put_user(SIGCHLD, &infop->si_signo);
+		if (!retval)
+			retval = put_user(0, &infop->si_errno);
+		if (!retval)
+			retval = put_user((short)why, &infop->si_code);
+		if (!retval)
+			retval = put_user(pid, &infop->si_pid);
+		if (!retval)
+			retval = put_user(uid, &infop->si_uid);
+		if (!retval)
+			retval = put_user(status, &infop->si_status);
+	}
 	if (!retval)
 		retval = pid;
 	return retval;

@@ -1727,35 +1729,12 @@ repeat:
 end:
 	current->state = TASK_RUNNING;
 	remove_wait_queue(&current->signal->wait_chldexit,&wait);
-	if (infop) {
-		if (retval > 0)
-			retval = 0;
-		else {
-			/*
-			 * For a WNOHANG return, clear out all the fields
-			 * we would set so the user can easily tell the
-			 * difference.
-			 */
-			if (!retval)
-				retval = put_user(0, &infop->si_signo);
-			if (!retval)
-				retval = put_user(0, &infop->si_errno);
-			if (!retval)
-				retval = put_user(0, &infop->si_code);
-			if (!retval)
-				retval = put_user(0, &infop->si_pid);
-			if (!retval)
-				retval = put_user(0, &infop->si_uid);
-			if (!retval)
-				retval = put_user(0, &infop->si_status);
-		}
-	}
 	return retval;
 }
 
-asmlinkage long sys_waitid(int which, pid_t upid,
-			   struct siginfo __user *infop, int options,
-			   struct rusage __user *ru)
+long do_waitid(int which, pid_t upid,
+	       struct siginfo __user *infop, int options,
+	       struct rusage __user *ru)
 {
 	struct pid *pid = NULL;
 	enum pid_type type;

@@ -1789,6 +1768,39 @@ asmlinkage long sys_waitid(int which, pid_t upid,
 	ret = do_wait(type, pid, options, infop, NULL, ru);
 	put_pid(pid);
 
+	return ret;
+}
+
+asmlinkage long sys_waitid(int which, pid_t upid,
+			   struct siginfo __user *infop, int options,
+			   struct rusage __user *ru)
+{
+	long ret;
+
+	ret = do_waitid(which, upid, infop, options, ru);
+
+	if (ret > 0)
+		ret = 0;
+	else {
+		/*
+		 * For a WNOHANG return, clear out all the fields
+		 * we would set so the user can easily tell the
+		 * difference.
+		 */
+		if (!ret)
+			ret = put_user(0, &infop->si_signo);

(Cc:-ed a few more folks who might be interested in this)

Quoted 7 lines · Casey Dahlin

Original description:

Linux now exposes signals, timers, and events via file descriptors through signalfd, timerfd, and eventfd. This means programmers can use a single select/[e]poll call to monitor all change in their program. This patch aims to expose child death via the same mechanism.

Quoted 6 lines

waitfd provides a file descriptor out of which may be read a series of siginfo_t objects describing child death. A child process is reaped as soon as its information is read. This means child monitoring too can be performed with that same poll call.

Patch is against v2.6.28

The principle looks sound and acceptable - to complete the epoll mechanism to all event sources in the system. There's a few small (mostly stylistic) details though:

Quoted 3 lines

index 0000000..0155a83

fs/waitfd.c new file +0 -0

fs/waitfd.c line 22

Quoted 6 lines

fs/waitfd.c new file +6 -0

+#include <linux/anon_inodes.h>
+#include <linux/syscalls.h>
+
+long do_waitid(int which, pid_t upid,
+	       struct siginfo __user *infop, int options,
+	       struct rusage __user *ru);

please move this prototype into sched.h.

fs/waitfd.c line 28

Quoted 5 lines

fs/waitfd.c new file +5 -0

+struct waitfd_ctx {
+	int ops;
+	int which;
+	pid_t upid;
+};

please align structure fields vertically, similarly to how you aligned other definitions in your patch. Something like:

fs/waitfd.c line 28

Quoted 5 lines

fs/waitfd.c new file +5 -0

+struct waitfd_ctx {
+	int	ops;
+	int	which;
+	pid_t	upid;
+};

(otherwise it looks all a bit too crammed together)

Quoted 42 lines

fs/waitfd.c new file +42 -0

+static int waitfd_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
+static unsigned int waitfd_poll(struct file *file, poll_table *wait)
+{
+	struct waitfd_ctx *ctx = file->private_data;
+	long value;
+
+	poll_wait(file, &current->signal->wait_chldexit, wait);
+
+	value = do_waitid(ctx->which, ctx->upid, NULL,
+			   ctx->ops | WNOHANG | WNOWAIT, NULL);
+	if (value > 0 || value == -ECHILD)
+		return POLLIN;
+
+	return 0;
+}
+
+/*
+ * Returns a multiple of the size of a struct siginfo, or a negative
+ * error code. The "count" parameter must be at least sizeof(struct siginfo)
+ */
+static ssize_t waitfd_read(struct file *file, char __user *buf, size_t count,
+			     loff_t *ppos)
+{
+	struct waitfd_ctx *ctx = file->private_data;
+	struct siginfo __user *info_addr = (struct siginfo *)buf;
+	int flags = ctx->ops;
+	ssize_t ret, total = 0;
+
+	count /= sizeof(struct siginfo);
+	if (!count)
+		return -EINVAL;
+
+	do {
+		ret = do_waitid(ctx->which, ctx->upid, info_addr, flags, NULL);
+		if (ret == 0)
+			ret = -EAGAIN;
+		if (ret == -ECHILD)

fs/waitfd.c line 81

Quoted 10 lines

fs/waitfd.c new file +10 -0

+			ret = 0;
+		if (ret <= 0)
+			break;
+
+		info_addr++;
+		total += sizeof(struct siginfo);
+		flags |= WNOHANG;
+	} while (--count);
+
+	return total ? total: ret;

please use symmetric spacing, i.e.:

Quoted 11 lines

fs/waitfd.c new file +10 -0

+	return total ? total : ret;
 
+}
+
+static const struct file_operations waitfd_fops = {
+	.release	= waitfd_release,
+	.poll		= waitfd_poll,
+	.read		= waitfd_read,
+};
+
+asmlinkage long sys_waitfd(int which, pid_t upid, int options, int unused)

fs/waitfd.c line 96

Quoted 6 lines

fs/waitfd.c new file +6 -0

+{
+	int ufd;
+	struct waitfd_ctx *ctx;
+
+	/* Just to make sure we don't end up with a sys_waitfd4 */
+	(void)unused;

looks a bit silly ...

Quoted 26 lines

fs/waitfd.c new file +21 -0

+
+	if (options & ~(WNOHANG|WEXITED|WSTOPPED|WCONTINUED))
+		return -EINVAL;
+	if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
+		return -EINVAL;
+
+	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->ops = options;
+	ctx->upid = upid;
+	ctx->which = which;
+
+	ufd = anon_inode_getfd("[waitfd]", &waitfd_fops, ctx,
+			       (options & WNOHANG) ? O_NONBLOCK : 0);
+	if (ufd < 0)
+		kfree(ctx);
+
+	return ufd;
+}

init/Kconfig +0 -0

index f763762..bc34871 100644

init/Kconfig config EPOLL · line 691

Quoted 9 lines

init/Kconfig +6 -0

@@ -683,6 +683,16 @@ config EPOLL
 	  Disabling this option will cause the kernel to be built without
 	  support for epoll family of system calls.
 
+config WAITFD
+	bool "Enable waitfd() system call" if EMBEDDED
+	select ANON_INODES
+	default y
+	help
+	  Enable the waitfd() system call that allows receving child state

typo.

Quoted 108 lines

init/Kconfig +4 -0

+	  changes from a file descriptor.
+
+	  If unsure, say Y.
+
 config SIGNALFD
 	bool "Enable signalfd() system call" if EMBEDDED
 	select ANON_INODES

kernel/exit.c +38 -38

index 2d8be7e..b53e8ba 100644
@@ -1233,18 +1233,20 @@ static int wait_noreap_copyout(struct task_struct *p, pid_t pid, uid_t uid,
 	int retval = rusagep ? getrusage(p, RUSAGE_BOTH, rusagep) : 0;
 
 	put_task_struct(p);
-	if (!retval)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval)
-		retval = put_user((short)why, &infop->si_code);
-	if (!retval)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval)
-		retval = put_user(uid, &infop->si_uid);
-	if (!retval)
-		retval = put_user(status, &infop->si_status);
+	if (infop) {
+		if (!retval)
+			retval = put_user(SIGCHLD, &infop->si_signo);
+		if (!retval)
+			retval = put_user(0, &infop->si_errno);
+		if (!retval)
+			retval = put_user((short)why, &infop->si_code);
+		if (!retval)
+			retval = put_user(pid, &infop->si_pid);
+		if (!retval)
+			retval = put_user(uid, &infop->si_uid);
+		if (!retval)
+			retval = put_user(status, &infop->si_status);
+	}
 	if (!retval)
 		retval = pid;
 	return retval;

@@ -1727,35 +1729,12 @@ repeat:
 end:
 	current->state = TASK_RUNNING;
 	remove_wait_queue(&current->signal->wait_chldexit,&wait);
-	if (infop) {
-		if (retval > 0)
-			retval = 0;
-		else {
-			/*
-			 * For a WNOHANG return, clear out all the fields
-			 * we would set so the user can easily tell the
-			 * difference.
-			 */
-			if (!retval)
-				retval = put_user(0, &infop->si_signo);
-			if (!retval)
-				retval = put_user(0, &infop->si_errno);
-			if (!retval)
-				retval = put_user(0, &infop->si_code);
-			if (!retval)
-				retval = put_user(0, &infop->si_pid);
-			if (!retval)
-				retval = put_user(0, &infop->si_uid);
-			if (!retval)
-				retval = put_user(0, &infop->si_status);
-		}
-	}
 	return retval;
 }
 
-asmlinkage long sys_waitid(int which, pid_t upid,
-			   struct siginfo __user *infop, int options,
-			   struct rusage __user *ru)
+long do_waitid(int which, pid_t upid,
+	       struct siginfo __user *infop, int options,
+	       struct rusage __user *ru)
 {
 	struct pid *pid = NULL;
 	enum pid_type type;

@@ -1789,6 +1768,39 @@ asmlinkage long sys_waitid(int which, pid_t upid,
 	ret = do_wait(type, pid, options, infop, NULL, ru);
 	put_pid(pid);
 
+	return ret;
+}
+
+asmlinkage long sys_waitid(int which, pid_t upid,
+			   struct siginfo __user *infop, int options,
+			   struct rusage __user *ru)
+{
+	long ret;
+
+	ret = do_waitid(which, upid, infop, options, ru);
+
+	if (ret > 0)
+		ret = 0;
+	else {
+		/*
+		 * For a WNOHANG return, clear out all the fields
+		 * we would set so the user can easily tell the
+		 * difference.
+		 */
+		if (!ret)
+			ret = put_user(0, &infop->si_signo);

kernel/exit.c asmlinkage long sys_waitid(int which, pid_t upid, · line 1801

Quoted 10 lines

kernel/exit.c +10 -0

+		if (!ret)
+			ret = put_user(0, &infop->si_errno);
+		if (!ret)
+			ret = put_user(0, &infop->si_code);
+		if (!ret)
+			ret = put_user(0, &infop->si_pid);
+		if (!ret)
+			ret = put_user(0, &infop->si_uid);
+		if (!ret)
+			ret = put_user(0, &infop->si_status);

even if this just moves existing code around, if we touch this, it would be far cleaner (and faster as well) to do what other bits of the signal code do:

kernel/exit.c asmlinkage long sys_waitid(int which, pid_t upid,

Quoted 6 lines

kernel/exit.c +6 -0

+		ret  = put_user(0, &infop->si_signo);
+		ret |= put_user(0, &infop->si_errno);
+		ret |= put_user(0, &infop->si_code);
+		ret |= put_user(0, &infop->si_pid);
+		ret |= put_user(0, &infop->si_uid);
+		ret |= put_user(0, &infop->si_status);

Since put_user() can only return -EFAULT or zero.

(same for wait_noreap_copyout())

Quoted 9 lines

fs/waitfd.c line 96

Quoted 7 lines

fs/waitfd.c new file +7 -0

+asmlinkage long sys_waitfd(int which, pid_t upid, int options, int unused)
+{
+	int ufd;
+	struct waitfd_ctx *ctx;
+
+	/* Just to make sure we don't end up with a sys_waitfd4 */
+	(void)unused;

looks a bit silly ...

Do you mean the principle of having an unused argument around for future use or the cast to void? The cast to void is there to suppress the "Waning: unused argument" messages and make gcc happy.

(Cc:-ed a few more folks who might be interested in this)

fs/waitfd.c line 96

Quoted 7 lines · Casey Dahlin

fs/waitfd.c new file +7 -0

+asmlinkage long sys_waitfd(int which, pid_t upid, int options, int unused)
+{
+	int ufd;
+	struct waitfd_ctx *ctx;
+
+	/* Just to make sure we don't end up with a sys_waitfd4 */
+	(void)unused;

looks a bit silly ...

Quoted 6 lines

fs/waitfd.c new file +6 -0

+
+	if (options & ~(WNOHANG|WEXITED|WSTOPPED|WCONTINUED))
+		return -EINVAL;
+	if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
+		return -EINVAL;
+

fs/waitfd.c line 112

Quoted 10 lines

fs/waitfd.c new file +10 -0

+	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->ops = options;
+	ctx->upid = upid;
+	ctx->which = which;
+
+	ufd = anon_inode_getfd("[waitfd]", &waitfd_fops, ctx,
+			       (options & WNOHANG) ? O_NONBLOCK : 0);

Quoted 15 lines · Ingo Molnar

(Cc:-ed a few more folks who might be interested in this)

fs/waitfd.c line 96

Quoted 7 lines · Casey Dahlin

fs/waitfd.c new file +7 -0

+asmlinkage long sys_waitfd(int which, pid_t upid, int options, int unused)
+{
+	int ufd;
+	struct waitfd_ctx *ctx;
+
+	/* Just to make sure we don't end up with a sys_waitfd4 */
+	(void)unused;

looks a bit silly ...

Quoted 16 lines

Quoted 6 lines

fs/waitfd.c new file +6 -0

+
+	if (options & ~(WNOHANG|WEXITED|WSTOPPED|WCONTINUED))
+		return -EINVAL;
+	if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
+		return -EINVAL;
+

fs/waitfd.c line 112

Quoted 10 lines

fs/waitfd.c new file +10 -0

+	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->ops = options;
+	ctx->upid = upid;
+	ctx->which = which;
+
+	ufd = anon_inode_getfd("[waitfd]", &waitfd_fops, ctx,
+			       (options & WNOHANG) ? O_NONBLOCK : 0);

minor nit...

Please note that unlike other sys_...fd() syscalls, sys_waitfd() doesn't allow to pass O_CLOEXEC. Looks like we need a separate "flags" argument...

Also, ioctl(FIONBIO) or fcntl(O_NONBLOCK) have no effect on waitfd, not very good.

I'd suggest to remove WNOHANG from waitfd_ctx->ops and treat (->f_flags & O_NONBLOCK) as WNOHANG.

(can't resist, ->ops is not the best name ;)

minor nit...

Please note that unlike other sys_...fd() syscalls, sys_waitfd() doesn't allow to pass O_CLOEXEC. Looks like we need a separate "flags" argument...

Also, ioctl(FIONBIO) or fcntl(O_NONBLOCK) have no effect on waitfd, not very good.

I'd suggest to remove WNOHANG from waitfd_ctx->ops and treat (->f_flags & O_NONBLOCK) as WNOHANG.

(can't resist, ->ops is not the best name ;)

Quoted 5 lines · Oleg Nesterov

minor nit...

Please note that unlike other sys_...fd() syscalls, sys_waitfd() doesn't allow to pass O_CLOEXEC. Looks like we need a separate "flags" argument...

I said this even during the first post. Not a minor nit unless we want to have a sys_waitfd2 soon after.

Please note that unlike other sys_...fd() syscalls, sys_waitfd() doesn't allow to pass O_CLOEXEC. Looks like we need a separate "flags" argument...

Also, ioctl(FIONBIO) or fcntl(O_NONBLOCK) have no effect on waitfd, not very good.

I'd suggest to remove WNOHANG from waitfd_ctx->ops and treat (->f_flags & O_NONBLOCK) as WNOHANG.

(can't resist, ->ops is not the best name ;)

Quoted 4 lines · Oleg Nesterov

Please note that unlike other sys_...fd() syscalls, sys_waitfd() doesn't allow to pass O_CLOEXEC. Looks like we need a separate "flags" argument...

Quoted 9 lines

Also, ioctl(FIONBIO) or fcntl(O_NONBLOCK) have no effect on waitfd, not very good.

I'd suggest to remove WNOHANG from waitfd_ctx->ops and treat (->f_flags & O_NONBLOCK) as WNOHANG.

(can't resist, ->ops is not the best name ;)

Definitely agree here, waitfd() doesn't need WNOHANG - we already have ONONBLOCK.

That also solves one of the strangest behaves of waitid when you use WNOHANG (it returns zero and you have to check whether it changed the struct), now you just read() - if no child you get EAGAIN, if a child you read a struct.

Scott

From the perspective of waitfd, the only difference between WNOHANG and O_NONBLOCK is which argument you put the flags in. The API should only support one or the other, but internally they would imply the same thing.

Quoted 5 lines · Casey Dahlin

From the perspective of waitfd, the only difference between WNOHANG and O_NONBLOCK is which argument you put the flags in. The API should only support one or the other, but internally they would imply the same thing.

Well, you get O_NONBLOCK for free by having a file descriptor; and you can't turn off people trying to turn it on/off with fcntl() - so you may as well just use that, no? :-)

Scott

Quoted 18 lines · Scott James Remnant

Quoted 4 lines · Oleg Nesterov

Please note that unlike other sys_...fd() syscalls, sys_waitfd() doesn't allow to pass O_CLOEXEC. Looks like we need a separate "flags" argument...

Quoted 9 lines

Also, ioctl(FIONBIO) or fcntl(O_NONBLOCK) have no effect on waitfd, not very good.

I'd suggest to remove WNOHANG from waitfd_ctx->ops and treat (->f_flags & O_NONBLOCK) as WNOHANG.

(can't resist, ->ops is not the best name ;)

Definitely agree here, waitfd() doesn't need WNOHANG - we already have ONONBLOCK.

Quoted 5 lines

That also solves one of the strangest behaves of waitid when you use WNOHANG (it returns zero and you have to check whether it changed the struct), now you just read() - if no child you get EAGAIN, if a child you read a struct.

From the perspective of waitfd, the only difference between WNOHANG and O_NONBLOCK is which argument you put the flags in.

Quoted 28 lines · Casey Dahlin

Quoted 26 lines · Scott James Remnant

Quoted 4 lines · Oleg Nesterov

Please note that unlike other sys_...fd() syscalls, sys_waitfd() doesn't allow to pass O_CLOEXEC. Looks like we need a separate "flags" argument...

Quoted 10 lines

Also, ioctl(FIONBIO) or fcntl(O_NONBLOCK) have no effect on waitfd, not very good.

I'd suggest to remove WNOHANG from waitfd_ctx->ops and treat (->f_flags & O_NONBLOCK) as WNOHANG.

(can't resist, ->ops is not the best name ;)

Definitely agree here, waitfd() doesn't need WNOHANG - we already have ONONBLOCK.

That also solves one of the strangest behaves of waitid when you use WNOHANG (it returns zero and you have to check whether it changed the struct), now you just read() - if no child you get EAGAIN, if a child you read a struct.

Quoted 3 lines

From the perspective of waitfd, the only difference between WNOHANG and O_NONBLOCK is which argument you put the flags in.

No. Please see the note about ioctl/fcntl above.

Oleg.

Quoted 3 lines · Oleg Nesterov

Quoted 2 lines

From the perspective of waitfd, the only difference between WNOHANG and O_NONBLOCK is which argument you put the flags in.

Quoted 4 lines

No. Please see the note about ioctl/fcntl above.

Oleg.

Yes but the actual waitfd call could simply set O_NONBLOCK on the descriptor when it receive WNOHANG in the flags, and read the descriptor flags going forward.

New syscall should have gone to linux-api, I think.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

Quoted 7 lines · Roland McGrath

New syscall should have gone to linux-api, I think.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

hm, i think it's cleaner conceptually than trying to wrap this into signalfd. Since we already have:

  #define __NR_signalfd           321
  #define __NR_timerfd_create     322
  #define __NR_timerfd_settime    325
  #define __NR_timerfd_gettime    326
  #define __NR_signalfd4          327

is one more really such an issue?

Quoted 20 lines · Ingo Molnar

Quoted 7 lines · Roland McGrath

New syscall should have gone to linux-api, I think.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

hm, i think it's cleaner conceptually than trying to wrap this into signalfd. Since we already have:

  #define __NR_signalfd           321
  #define __NR_timerfd_create     322
  #define __NR_timerfd_settime    325
  #define __NR_timerfd_gettime    326
  #define __NR_signalfd4          327

Quoted 1 line

is one more really such an issue?

And what did eventfd do to you? :)

I partially agree with Roland (and I stated this during Casey's first post), this can be achieved in a not too troublesome way already. A new dedicated interface is easier for the challenged userspace coder, but I dunno if it's worth the new code (although it does have little footprint). Both ways are fine from my POV.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

Quoted 5 lines · Roland McGrath

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

Since waitfd shouldn't consume the child termination notification waitfd should be more widely usable than the wait*() interfaces.

I.e., it's not necessary to restrict the use to parents. Any process with the same UID should be allowed to call waitfd. This would allow some new user cases.

Quoted 2 lines

Since waitfd shouldn't consume the child termination notification waitfd should be more widely usable than the wait*() interfaces.

Quoted 2 lines

Quoted 2 lines

Since waitfd shouldn't consume the child termination notification waitfd should be more widely usable than the wait*() interfaces.

waitid can be used that way with WNOWAIT.

Quoted 4 lines · Roland McGrath

Quoted 2 lines

Quoted 2 lines

Since waitfd shouldn't consume the child termination notification waitfd should be more widely usable than the wait*() interfaces.

waitid can be used that way with WNOWAIT.

Yes, but waitfd does not have this flag. The reason being waitfd just calls waitid internally, and there is no guarantee (afaik) that calling waitid with WNOWAIT multiple times in succession will yield different results each time. This breaks the streaming behavior of the descriptor.

Btw. It is not that I am trying to argue against sys_waitfd(), but do you have the "real life" example when it can be useful? Yes, poll(). But we have signalfd. SIGCHLD is not rt signal, but afaics this is not the problem actually. Just curious.

Quoted 5 lines · Oleg Nesterov

Btw. It is not that I am trying to argue against sys_waitfd(), but do you have the "real life" example when it can be useful? Yes, poll(). But we have signalfd. SIGCHLD is not rt signal, but afaics this is not the problem actually. Just curious.

signalfd() can't currently be made to work in the way you describe.

I am not sure we are talking about the same thing, but afaics poll() + signalfd can work to (say) reap the childs. Actually, ppoll() alone is enough.

Quoted 4 lines · Oleg Nesterov

I am not sure we are talking about the same thing, but afaics poll() + signalfd can work to (say) reap the childs. Actually, ppoll() alone is enough.

Last time I checked, ppoll() was not actually implemented across all architectures in a manner that solved the race it was intended to solve.

Quoted 10 lines · Scott James Remnant

Quoted 4 lines · Oleg Nesterov

I am not sure we are talking about the same thing, but afaics poll() + signalfd can work to (say) reap the childs. Actually, ppoll() alone is enough.

Last time I checked, ppoll() was not actually implemented across all architectures in a manner that solved the race it was intended to solve.

As I said, this is imho unfair. But I mentioned ppol() "just in case".

My questiong was why do you think that "signalfd() can't currently be made to work in the way you describe". You have dropped this part to change the topic?

Quoted 3 lines · Roland McGrath

Quoted 2 lines

Quoted 2 lines

Since waitfd shouldn't consume the child termination notification waitfd should be more widely usable than the wait*() interfaces.

Quoted 1 line

waitid can be used that way with WNOWAIT.

Yes, but waitfd does not have this flag. The reason being waitfd just calls waitid internally, and there is no guarantee (afaik) that calling waitid with WNOWAIT multiple times in succession will yield different results each time. This breaks the streaming behavior of the descriptor.

Quoted 5 lines · Casey Dahlin

Quoted 3 lines · Roland McGrath

Quoted 3 lines

Quoted 3 lines

Since waitfd shouldn't consume the child termination notification waitfd should be more widely usable than the wait*() interfaces.

Quoted 9 lines

Quoted 2 lines

waitid can be used that way with WNOWAIT.

Yes, but waitfd does not have this flag. The reason being waitfd just calls waitid internally, and there is no guarantee (afaik) that calling waitid with WNOWAIT multiple times in succession will yield different results each time. This breaks the streaming behavior of the descriptor.

This would definitely be a Nice To Have though!

Being able to use waitid() on another process of the same uid, with WNOHANG, in a streaming fashion would be a *very* cool thing.

Scott

New syscall should have gone to linux-api, I think.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

Thanks,
Roland
--
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/

New syscall should have gone to linux-api, I think.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

New syscall should have gone to linux-api, I think.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

Quoted 8 lines · Roland McGrath

New syscall should have gone to linux-api, I think.

Do we really need another one for this? How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD? It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN). But compared to adding a syscall we don't really have to add, maybe better.

This wouldn't help the init daemon case:

- the exit_signal is set on the child, not on the parent.

While the init daemon could clone() every new process and set exit_signal, this would not be set for processes reparented to init.

Even if we had a new syscall to change the exit_signal of a given process, *and* had the init reparent notification patch, this still wouldn't be sufficient; you'd have a race condition between the time you were notified of the reparent, and the time you set exit_signal, in which the child could die.

Since exit_signal is always reset to SIGCHLD before reparenting, this could be done by resetting it to a different signal; but at this point we're getting into a rather twisty method full of traps.

- exit_signal is reset to SIGCHLD on exec().

Pretty much a plan-killer ;)

I can't understand why should we change ->exit_signal if we want to use signalfd. Yes, SIGCHLD is not rt. So what?

We do not need multiple signals in queue if we want to reap multiple zombies. Once we have a single SIGCHLD (reported by signalfd or whatever) we can do do_wait(WNOHANG) in a loop.

Quoted 3 lines · Oleg Nesterov

I can't understand why should we change ->exit_signal if we want to use signalfd. Yes, SIGCHLD is not rt. So what?

Quoted 4 lines

We do not need multiple signals in queue if we want to reap multiple zombies. Once we have a single SIGCHLD (reported by signalfd or whatever) we can do do_wait(WNOHANG) in a loop.

Well, a good reason why is that it makes things much easier to do in userspace.

You may as well ask why we have signalfd() at all, and what was wrong with sigaction() and ordinary signal handlers? Well, lots of things

So let's compare userspace code for trying to reap children using signalfd();

First, what we have today:

[...snip the code...]

Pros:
- code exists today

Cons:
- having siginfo_t returned by read() is pointless, we can't use it

- double loop isn't pretty

- strange waitid() API in case of WNOHANG and no child

- incompatible structures for signalfd()'s read result and waitid(), despite being logically the same structure! :-/

- can't simultaneously clear pending signal and wait, so we always have to go back round the main loop if a child dies after the read()

Since there's no point listening to SIGCHLD, it's a complete no-op, we don't respond to it at all. We only need to use it to wake up the main loop.

The wait() loop tends to be at the bottom of the main loop somewhere, completely outside of the fd processing.

Now, what if signalfd() would always queue pending signals even if they're non-RT?

So what about
waitfd()

Quoted 4 lines

I am not sure we are talking about the same thing, but afaics poll() + signalfd can work to (say) reap the childs. Actually, ppoll() alone is enough.

Last time I checked, ppoll() was not actually implemented across all architectures in a manner that solved the race it was intended to solve.

I'd be delighted to learn that this had been fixed? :-)

Quoted 10 lines · Scott James Remnant

Quoted 7 lines · Oleg Nesterov

I can't understand why should we change ->exit_signal if we want to use signalfd. Yes, SIGCHLD is not rt. So what?

We do not need multiple signals in queue if we want to reap multiple zombies. Once we have a single SIGCHLD (reported by signalfd or whatever) we can do do_wait(WNOHANG) in a loop.

Quoted 2 lines

Well, a good reason why is that it makes things much easier to do in userspace.

I never argued with this. And, let me repeat. I am not arguing against waitfd! Actually, I always try to avoid the "do we need this feature" discussions.

What I disagree with is that waitfd adds the functionality which does not exists currently.

Quoted 2 lines

Cons:
- having siginfo_t returned by read() is pointless, we can't use it

Indeed. We use read() only to wait for the signal death.

Quoted 2 lines

- can't simultaneously clear pending signal and wait, so we always have to go back round the main loop if a child dies after the read()

Can't understand... waitfd doesn't clear the signal too?

And you forget to mention another drwaback with the current code:
a lot of pathetic comments ;)

Quoted 2 lines

The wait() loop tends to be at the bottom of the main loop somewhere, completely outside of the fd processing.

Huh.

Quoted 2 lines

Now, what if signalfd() would always queue pending signals even if they're non-RT?

Well, I think this is off-topic, and more importantly I don't think this change is possible.

Quoted 2 lines

So what about
waitfd()

Yes, the user-space code (for this particular artificial example) becomes simpler. Following this logic, let's add sys_copyfile() to kernel? From time to time I regret we don't have it...

(from another thread)

Quoted 4 lines · Oleg Nesterov

I never argued with this. And, let me repeat. I am not arguing against waitfd! Actually, I always try to avoid the "do we need this feature" discussions.

Unless I'm misinterpreting you, you're saying that you don't understand why we should change any current behaviour? My post is attempting to illustrate why we should.

Quoted 3 lines

What I disagree with is that waitfd adds the functionality which does not exists currently.

I'm not saying that it doesn't at all; in fact I gave an example of how you implement the exact same functionality today.

In fact, because main loops use select()/poll(), for the SIGCHLD case you'd never use signalfd() at all!

Unless I'm missing something, the following two examples are identical in behaviour:

using signalfd:
...
using pselect:

But the pselect() version is neater. Which is why I started the previous reply off with "why have signalfd() at all?"

One of them was attempting to explain what you don't understand here, I'll try and be more verbose...
...
~~Calling waitpid() does not clear the pending signal.~~

This is the important bit.

If a further process dies while we're inside the waitpid() loop, we will most likely reap that straight away. But this does not clear the pending signal. The main loop will be woken up again, even though it does not need to be.

Thus:

- child process #1 dies
- main loop woken up by SIGCHLD
- pending status of signal cleared
- enter wait loop
- child process #2 dies
- SIGCHLD pending again
- waitpid() called first time, child process #1 reaped
- waitpid() called second time, child process #2 reaped (SIGCHLD still pending)
- waitpid() called third time, no child processes remain

- exit wait loop
- back to top of main loop, immediately woken up by pending SIGCHLD
- pending status of signal cleared
- enter wait loop
- waitpid() called first time, but no child processes remain (we reaped it last time round)
- exit wait loop
- back to top of main loop, sleep

- SIGCHLD not pending, but waitpid() will not block

This is true in all example usage; after you've called the read() on the signalfd - or the pselect() has woken, SIGCHLD is probably no longer pending but waitpid() will not block

Compare with select() behaviour; if you fail to read() from the fd, select() wakes up yet again

- SIGCHLD pending, but waitpid() will block

This is true if you exhaust the wait queue in a loop,

All SIGCHLD is useful for is to get your main loop out of select()/poll(); you must always exhaust the wait queue every time you have woken up.

A more orthogonal example would be pselect(). That implemented, in the kernel, a syscall that it actually wasn't possible to implement in userspace

The argument for waitfd() or similar in the kernel is because there are races in userspace that we can't solve.

Quoted 12 lines

Thus:

- child process #1 dies
- main loop woken up by SIGCHLD
- pending status of signal cleared
- enter wait loop
- child process #2 dies
- SIGCHLD pending again
- waitpid() called first time, child process #1 reaped
- waitpid() called second time, child process #2 reaped (SIGCHLD still pending)
- waitpid() called third time, no child processes remain

Quoted 8 lines

- exit wait loop
- back to top of main loop, immediately woken up by pending SIGCHLD
- pending status of signal cleared
- enter wait loop
- waitpid() called first time, but no child processes remain (we reaped it last time round)
- exit wait loop
- back to top of main loop, sleep

Scott, I don't really understand why are you trying to explain this all to me. I do understand this. At least I hope ;)

That's pretty neat, much nicer than what we had before. So what about waitfd() [I think I've slightly changed Casey's API here, or he changed my proposed one <g>]?

First, what we have today:

	sigemptyset (&mask);
	sigaddset (&mask, SIGCHLD);

	/* Block normal delivery and receive by sigfd instead */
	sigprocmask (SIG_BLOCK, &mask, NULL);
	sigfd = signalfd (-1, &mask, 0);

	for (;;) {
		read (sigfd, &fd_siginfo, sizeof siginfo);

		/* throw away fd_siginfo, we're reading SIGCHLD and
		 * can't use it :-(
		 */

		/* SIGCHLD means _at_least_one_ child is pending, there
		 * may be more; so we have to loop AND expect to find
		 * nothing
		 */
		for (;;) {
			/* ARGH! waitid returns 0 with WNOHANG if there
			 * are no children.
			 *
			 * AND the structure, despite being logically
			 * the same, isn't the same as the signalfd
			 * one :-/
			 */
			memset (&w_siginfo, 0, sizeof w_siginfo);

			waitid (P_ALL, 0, &w_siginfo,
				WEXITED | WNOHANG);

			/* Did we find anything? */
			if (! w_siginfo.si_pid)

				break;

			/* NOW we have the siginfo_t for a recently
			 * deceased process
			 */

			mourn (&w_siginfo);
		}

Quoted 20 lines · Scott James Remnant

Quoted 6 lines · Roland McGrath

Do we really need another one for this?  How about using signalfd plus setting the child's exit_signal to a queuing (SIGRTMIN+n) signal instead of SIGCHLD?  It's slightly more magical for the userland process to know to do that (fork -> clone SIGRTMIN).  But compared to adding a syscall we don't really have to add, maybe better.

This wouldn't help the init daemon case:

- the exit_signal is set on the child, not on the parent.

  While the init daemon could clone() every new process and set exit_signal, this would not be set for processes reparented to init.

  Even if we had a new syscall to change the exit_signal of a given process, *and* had the init reparent notification patch, this still wouldn't be sufficient; you'd have a race condition between the time you were notified of the reparent, and the time you set exit_signal, in which the child could die.

Quoted 7 lines

  Since exit_signal is always reset to SIGCHLD before reparenting, this could be done by resetting it to a different signal; but at this point we're getting into a rather twisty method full of traps.

- exit_signal is reset to SIGCHLD on exec().

  Pretty much a plan-killer ;)

I can't understand why should we change ->exit_signal if we want to use signalfd. Yes, SIGCHLD is not rt. So what?

We do not need multiple signals in queue if we want to reap multiple zombies. Once we have a single SIGCHLD (reported by signalfd or whatever) we can do do_wait(WNOHANG) in a loop.

Confused.

Quoted 5 lines · Oleg Nesterov

We do not need multiple signals in queue if we want to reap multiple zombies. Once we have a single SIGCHLD (reported by signalfd or whatever) we can do do_wait(WNOHANG) in a loop.

Quoted 1 line

Confused.

I know I am terribly late for the party :)

"do_wait(WNOHANG) in a loop" is a performance problem.

Oleg, do you remember that strace bug when it was swamped with gazillions of stop notifications from a multithreaded task, then by dealing with them one-by-one it was causing unfairness and ultimately "this program never finishes when run under strace" bug?

And another typical nuisance that running multithreaded stuff under strace is much slower, even with -e option which limits the set of decoded syscalls?

Having waitfd would help both cases: strace can gulp a lot of waitpid notifications in one go, and batch process them.