Re: [RFC PATCH v2] waitfd

[Randy Dunlap <randy.dunlap@oracle.com>]

Casey Dahlin wrote:
> 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
> 
> diff --git a/arch/x86/include/asm/unistd_32.h
> b/arch/x86/include/asm/unistd_32.h
> index f2bba78..134d83c 100644
> --- a/arch/x86/include/asm/unistd_32.h
> +++ b/arch/x86/include/asm/unistd_32.h
> @@ -338,6 +338,7 @@
> #define __NR_dup3        330
> #define __NR_pipe2        331
> #define __NR_inotify_init1    332
> +#define __NR_waitfd        333
> 
> #ifdef __KERNEL__
> 
> diff --git a/arch/x86/include/asm/unistd_64.h
> b/arch/x86/include/asm/unistd_64.h
> index d2e415e..b28eb07 100644
> --- a/arch/x86/include/asm/unistd_64.h
> +++ b/arch/x86/include/asm/unistd_64.h
> @@ -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??

> 
> #ifndef __NO_STUBS
> diff --git a/arch/x86/kernel/syscall_table_32.S
> b/arch/x86/kernel/syscall_table_32.S
> index d44395f..c796a8b 100644
> --- a/arch/x86/kernel/syscall_table_32.S
> +++ b/arch/x86/kernel/syscall_table_32.S
> @@ -332,3 +332,4 @@ ENTRY(sys_call_table)
>     .long sys_dup3            /* 330 */
>     .long sys_pipe2
>     .long sys_inotify_init1
> +    .long sys_waitfd
> diff --git a/fs/Makefile b/fs/Makefile
> index d9f8afe..74c31fb 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -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
> diff --git a/fs/waitfd.c b/fs/waitfd.c
> new file mode 100644
> index 0000000..0155a83
> --- /dev/null
> +++ b/fs/waitfd.c
> @@ -0,0 +1,117 @@
> +/*
> + *  fs/waitfd.c
> + *
> + *  Copyright (C) 2008  Red Hat, Casey Dahlin <cdahlin@redhat.com>
> + *
> + *  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;
> +};
> +

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.

> +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)
> +            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.

> +}
> +
> +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;
> +
> +    if (options & ~(WNOHANG|WEXITED|WSTOPPED|WCONTINUED))
> +        return -EINVAL;
> +    if (!(options & (WEXITED|WSTOPPED|WCONTINUED)))
> +        return -EINVAL;

Use spaces around '|'.

> +
> +    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;
> +}
> diff --git a/init/Kconfig b/init/Kconfig
> index f763762..bc34871 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -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>.

> +      changes from a file descriptor.
> +
> +      If unsure, say Y.
> +
> config SIGNALFD
>     bool "Enable signalfd() system call" if EMBEDDED
>     select ANON_INODES
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 2d8be7e..b53e8ba 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -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);
> +        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;
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index e14a232..e8d4da6 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -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);


-- 
~Randy