On Fri, Apr 19, 2019 at 09:22:33PM +0200, Christian Brauner wrote:
On Fri, Apr 19, 2019 at 09:18:58PM +0200, Christian Brauner wrote:
On Fri, Apr 19, 2019 at 03:02:47PM -0400, Joel Fernandes wrote:
On Thu, Apr 18, 2019 at 07:26:44PM +0200, Christian Brauner wrote:
On April 18, 2019 7:23:38 PM GMT+02:00, Jann Horn jannh@google.com wrote:
On Wed, Apr 17, 2019 at 3:09 PM Oleg Nesterov oleg@redhat.com wrote:
On 04/16, Joel Fernandes wrote: > On Tue, Apr 16, 2019 at 02:04:31PM +0200, Oleg Nesterov wrote: > > > > Could you explain when it should return POLLIN? When the whole
process exits?
> > It returns POLLIN when the task is dead or doesn't exist anymore,
or when it
> is in a zombie state and there's no other thread in the thread
group.
IOW, when the whole thread group exits, so it can't be used to
monitor sub-threads.
just in case... speaking of this patch it doesn't modify
proc_tid_base_operations,
so you can't poll("/proc/sub-thread-tid") anyway, but iiuc you are
going to use
the anonymous file returned by CLONE_PIDFD ?
I don't think procfs works that way. /proc/sub-thread-tid has proc_tgid_base_operations despite not being a thread group leader. (Yes, that's kinda weird.) AFAICS the WARN_ON_ONCE() in this code can be hit trivially, and then the code will misbehave.
@Joel: I think you'll have to either rewrite this to explicitly bail out if you're dealing with a thread group leader, or make the code work for threads, too.
The latter case probably being preferred if this API is supposed to be useable for thread management in userspace.
At the moment, we are not planning to use this for sub-thread management. I am reworking this patch to only work on clone(2) pidfds which makes the above
Indeed and agreed.
discussion about /proc a bit unnecessary I think. Per the latest CLONE_PIDFD patches, CLONE_THREAD with pidfd is not supported.
Yes. We have no one asking for it right now and we can easily add this later.
Admittedly I haven't gotten around to reviewing the patches here yet completely. But one thing about using POLLIN. FreeBSD is using POLLHUP on process exit which I think is nice as well. How about returning POLLIN | POLLHUP on process exit? We already do things like this. For example, when you proxy between ttys. If the process that you're reading data from has exited and closed it's end you still can't usually simply exit because it might have still buffered data that you want to read. The way one can deal with this from userspace is that you can observe a (POLLHUP | POLLIN) event and you keep on reading until you only observe a POLLHUP without a POLLIN event at which point you know you have read all data. I like the semantics for pidfds as well as it would indicate:
- POLLHUP -> process has exited
or POLLRDHUP. The check you'd usually perform would probably be if ((revents & (POLLIN | POLLPRI)) > 0) && ((revents & (POLLHUP | POLLRDHUP)) > 0) /* keep on trying to read */
I guess you have that set of flags already suggested in another mail?
The code where this pattern is e.g. used is in drivers/tty/n_tty.c:
static __poll_t n_tty_poll(struct tty_struct *tty, struct file *file, poll_table *wait) { __poll_t mask = 0;
poll_wait(file, &tty->read_wait, wait); poll_wait(file, &tty->write_wait, wait); if (input_available_p(tty, 1)) mask |= EPOLLIN | EPOLLRDNORM; else { tty_buffer_flush_work(tty->port); if (input_available_p(tty, 1)) mask |= EPOLLIN | EPOLLRDNORM; } if (tty->packet && tty->link->ctrl_status) mask |= EPOLLPRI | EPOLLIN | EPOLLRDNORM; if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) mask |= EPOLLHUP; if (tty_hung_up_p(file)) mask |= EPOLLHUP; if (tty->ops->write && !tty_is_writelocked(tty) && tty_chars_in_buffer(tty) < WAKEUP_CHARS && tty_write_room(tty) > 0) mask |= EPOLLOUT | EPOLLWRNORM; return mask; }