waitfd: file descriptor to wait on child processes

prototype for a fourth: waitfd. It pretty much does what the name
suggests: reading from it yields a series of status ints (as would be written into the second argument of waitpid) for child processes that have changed state. It takes essentially the same arguments as waitpid (for now) and supports the same set of features.

Here is a more elegant suggestion - use epoll, inotify and friends fully on /proc process nodes.

Quoted 2 lines

prototype for a fourth: waitfd. It pretty much does what the name suggests: reading from it yields a series of status ints (as would be written into the second argument of waitpid) for child processes that have changed state. It takes essentially the same arguments as waitpid (for now) and supports the same set of features.

This propogates the fundamental braindamage of waitpid - the fact the notification only works on child process trees.

Here is a more elegant suggestion - use epoll, inotify and friends fully on /proc process nodes.

Alan

--
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/

Quoted 5 lines

This propogates the fundamental braindamage of waitpid - the fact the notification only works on child process trees.

Here is a more elegant suggestion - use epoll, inotify and friends fully on /proc process nodes.

Last I checked inotify was not supported in /proc, or at least most of it. What kind of work load is it to change that?

Quoted 6 lines

Quoted 6 lines

This propogates the fundamental braindamage of waitpid - the fact the notification only works on child process trees.

Here is a more elegant suggestion - use epoll, inotify and friends fully on /proc process nodes.

Quoted 2 lines

Last I checked inotify was not supported in /proc, or at least most of it. What kind of work load is it to change that?

I don't know but I think it would be the better approach to find it. That also separates notification of state to parents from the general problem of wanting to know when a service has died, which seems to be an ever more common point of interest on the desktop in particular.

File content change notification for /proc is hard because the contents don't exist in the normal way and get updated but can be done if there is a wait queue for the job. Actual changes to structure (new directories etc) is in part a similar problem but there are clear points already in existence when the proc nodes are created/destroyed and thus notification can occur.

This is essentially my first kernel patch, so be nice :)

Linux already has signalfd, timerfd, and eventfd to expose signals, timers, and events via a file descriptor. This patch is a working prototype for a
fourth: waitfd. It pretty much does what the name suggests: reading from it yields a series of status ints (as would be written into the second argument of waitpid) for child processes that have changed state. It takes essentially the same arguments as waitpid (for now) and supports the same set of features.

This is far from ready for merge. Some things that are wrong with it:
* Waitpid's argument scheme probably isn't the best for this. By default it makes it easiest to wait on a single child, which is not often useful in this case. Waiting on all children or children in a particular process group is possible, but not a particular, explicit set of children, which we probably want (and which will complicate the implementation significantly).
* The prototype for peek_waitpid is obviously in the wrong place, but I haven't found a good home for it.
* Waitid's semantics have slightly altered: passing NULL as the pointer to siginfo_t with WNOWAIT will now return successfully instead of throwing EFAULT. I don't know if that means I broke it or fixed it :)
* peek_waitpid may not be required at all now. I can probably trick sys_wait4 or sys_waitid into giving me what I want (or I could always just make do_wait non-static).

Please provide thoughts.

+/*
+ * Returns a multiple of the size of a "struct waitfd_siginfo", or a negative
+ * error code. The "count" parameter must be at least the size of a
+ * "struct waitfd_siginfo".
+ */

fs/waitfd.c new file +19 -0

+static ssize_t waitfd_read(struct file *file, char __user *buf, size_t count,
+                 loff_t *ppos)
+{
+    struct waitfd_ctx *ctx = file->private_data;
+    int __user *stat_addr = (int *)buf;
+    int nonblock = file->f_flags & O_NONBLOCK ? WNOHANG: 0;
+    ssize_t ret, total = 0;
+
+    count /= sizeof(int);
+    if (!count)
+        return -EINVAL;
+
+    do {
+        ret = sys_wait4(ctx->upid, stat_addr, ctx->ops | nonblock,
+                NULL);
+        if (ret == 0)
+            ret = -EAGAIN;
+        if (ret == -ECHILD)
+            ret = 0;

(quoted diff) +22 -0

+long peek_waitpid(pid_t upid, int options)
+{
+    struct pid *pid = NULL;
+    enum pid_type type;
+    long ret;
+
+    if (options & ~(WNOHANG|WUNTRACED|WCONTINUED|
+            __WNOTHREAD|__WCLONE|__WALL))
+        return -EINVAL;
+
+    options |= WNOHANG | WNOWAIT;
+
+    if (upid == -1)
+        type = PIDTYPE_MAX;
+    else if (upid < 0) {
+        type = PIDTYPE_PGID;
+        pid = find_get_pid(-upid);
+    } else if (upid == 0) {
+        type = PIDTYPE_PGID;
+        pid = get_pid(task_pgrp(current));
+    } else /* upid > 0 */ {
+        type = PIDTYPE_PID;

Quoted 24 lines · Casey Dahlin

This is essentially my first kernel patch, so be nice :)

Linux already has signalfd, timerfd, and eventfd to expose signals, timers, and events via a file descriptor. This patch is a working prototype for a
fourth: waitfd. It pretty much does what the name suggests: reading from it yields a series of status ints (as would be written into the second argument of waitpid) for child processes that have changed state. It takes essentially the same arguments as waitpid (for now) and supports the same set of features.

This is far from ready for merge. Some things that are wrong with it:
* Waitpid's argument scheme probably isn't the best for this. By default it makes it easiest to wait on a single child, which is not often useful in this case. Waiting on all children or children in a particular process group is possible, but not a particular, explicit set of children, which we probably want (and which will complicate the implementation significantly).
* The prototype for peek_waitpid is obviously in the wrong place, but I haven't found a good home for it.
* Waitid's semantics have slightly altered: passing NULL as the pointer to siginfo_t with WNOWAIT will now return successfully instead of throwing EFAULT. I don't know if that means I broke it or fixed it :)
* peek_waitpid may not be required at all now. I can probably trick sys_wait4 or sys_waitid into giving me what I want (or I could always just make do_wait non-static).

Quoted 2 lines

Please provide thoughts.

What's wrong in having a signalfd on SIGCHLD, than doing waitpid() once you get the signal? Do you have cases where this wouldn't be OK?

About the code, eventually, you really want to report the PID of the exited child, together with the status. So maybe a non-compat-requiring struct would be better to be returned by read(2). Also ...

Quoted 4 lines

fs/waitfd.c new file +3 -0

+static unsigned int waitfd_poll(struct file *file, poll_table *wait)
+{
+    struct waitfd_ctx *ctx = file->private_data;

fs/waitfd.c line 45

Quoted 10 lines

fs/waitfd.c new file +10 -0

+    long value;
+
+    poll_wait(file, &current->signal->wait_chldexit, wait);
+
+    value = peek_waitpid(ctx->upid, ctx->ops);
+    if (value > 0) {
+        return POLLIN;
+    } if (value == -ECHILD) {
+        return POLLIN;
+    }

Trust the compiler, it's pretty good in not having you to add Perl-like extra brackets ;) This also looks wierd:

	} if (value == -ECHILD) {

So maybe:

	return value > 0 || value == -ECHILD ? POLLIN: 0;

Quoted 9 lines

+/*
+ * Returns a multiple of the size of a "struct waitfd_siginfo", or a negative
+ * error code. The "count" parameter must be at least the size of a
+ * "struct waitfd_siginfo".
+ */

Really? ...

Quoted 25 lines

fs/waitfd.c new file +24 -0

+static ssize_t waitfd_read(struct file *file, char __user *buf, size_t count,
+                 loff_t *ppos)
+{
+    struct waitfd_ctx *ctx = file->private_data;
+    int __user *stat_addr = (int *)buf;
+    int nonblock = file->f_flags & O_NONBLOCK ? WNOHANG: 0;
+    ssize_t ret, total = 0;
+
+    count /= sizeof(int);
+    if (!count)
+        return -EINVAL;
+
+    do {
+        ret = sys_wait4(ctx->upid, stat_addr, ctx->ops | nonblock,
+                NULL);
+        if (ret == 0)
+            ret = -EAGAIN;
+        if (ret == -ECHILD)
+            ret = 0;
+        if (ret <= 0)
+            break;
+
+        stat_addr++;
+        total += sizeof(struct siginfo);

fs/waitfd.c line 83

Quoted 6 lines

fs/waitfd.c new file +5 -0

+        nonblock = WNOHANG;
+    } while (--count);
+
+    return total ? total: ret;
+}

... looks like you're returning a sequence of status ints, with wrong data size returned by read(2).

fs/waitfd.c line 91

Quoted 11 lines

fs/waitfd.c new file +8 -0

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

Please leave space for extra flags for fds, otherwise Uli will have to make another sys_waitfd3().

Quoted 28 lines

(quoted diff) +27 -0

+long peek_waitpid(pid_t upid, int options)
+{
+    struct pid *pid = NULL;
+    enum pid_type type;
+    long ret;
+
+    if (options & ~(WNOHANG|WUNTRACED|WCONTINUED|
+            __WNOTHREAD|__WCLONE|__WALL))
+        return -EINVAL;
+
+    options |= WNOHANG | WNOWAIT;
+
+    if (upid == -1)
+        type = PIDTYPE_MAX;
+    else if (upid < 0) {
+        type = PIDTYPE_PGID;
+        pid = find_get_pid(-upid);
+    } else if (upid == 0) {
+        type = PIDTYPE_PGID;
+        pid = get_pid(task_pgrp(current));
+    } else /* upid > 0 */ {
+        type = PIDTYPE_PID;
+        pid = find_get_pid(upid);
+    }
+
+    ret = do_wait(type, pid, options | WEXITED, NULL, NULL, NULL);
+    put_pid(pid);

Review

Quoted 6 lines

(quoted diff) +5 -0

+
+    /* avoid REGPARM breakage on x86: */
+    asmlinkage_protect(4, ret, upid, options);
+    return ret;
+}

Given that you blinded copied this from sys_wait4(), you may want to at least try to make sys_wait4() to re-use the new function.

Also, your patch does not apply to latest Linus tree. Which one was the base?

Quoted 7 lines · Casey Dahlin

Linux already has signalfd, timerfd, and eventfd to expose signals, timers, and events via a file descriptor. This patch is a working prototype for a
fourth: waitfd. It pretty much does what the name suggests: reading from it yields a series of status ints (as would be written into the second argument of waitpid) for child processes that have changed state. It takes essentially the same arguments as waitpid (for now) and supports the same set of features.

What's wrong in having a signalfd on SIGCHLD, than doing waitpid() once you get the signal?

Quoted 9 lines · Davide Libenzi

Quoted 7 lines · Casey Dahlin

Linux already has signalfd, timerfd, and eventfd to expose signals, timers, and events via a file descriptor. This patch is a working prototype for a
fourth: waitfd. It pretty much does what the name suggests: reading from it yields a series of status ints (as would be written into the second argument of waitpid) for child processes that have changed state. It takes essentially the same arguments as waitpid (for now) and supports the same set of features.

Quoted 3 lines

What's wrong in having a signalfd on SIGCHLD, than doing waitpid() once you get the signal?

Because SIGCHLD isn't a POSIX realtime signal, only one copy of it will be queued at any one time -- even with signalfd(), and even though they have different (useful) siginfo_t.

So if you have three children die in rapid succession, you only get the siginfo for the first one. Thus you still have to call waitid()/waitpid() in a loop, and wait on everything.

Could the fact that you don't get signalfd notification of the additional signals be considered a bug? Or possibly a useful additional feature?

If we were able to read all the queued SIGCHLD signals with signalfd (preserving the one pending only behaviour of ordinary delivery), then a loop like the following would be possible:

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

  sfd = signalfd (-1, &mask, 0);

  for (;;) {
    read (sfd, &fdsi, sizeof (struct signalfd_siginfo));

    waitpid (fdsi.ssi_pid, 0, 0);

}

And how about this?

	sfd = signalfd(SIGCHLD);

	for (;;) {
		poll(sfd, POLLIN);
		while ((pid = waitpid(0, &status, WNOHANG)) != -1)
			process_child_death(pid);
	}

Quoted 10 lines · Davide Libenzi

And how about this?

	sfd = signalfd(SIGCHLD);

	for (;;) {
		poll(sfd, POLLIN);
		while ((pid = waitpid(0, &status, WNOHANG)) != -1)
			process_child_death(pid);
	}

At this point, why have signalfd()'s read() return siginfo_t at all?

You have to discard the entire information since it's only ever the first signal that matched, all subsequent ones are thrown away.