| Message ID | 20190705095914.151056-2-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add terminal detach feature | expand |
On Fri, Jul 05, 2019 at 10:59:13AM +0100, Andre Przywara wrote: > Currently when kvmtool is creating a pseudoterminal (--tty x), the > terminal thread will consume 100% of its CPU time as long as no slave > is connected to the other end. This is due to the fact that poll() > unconditonally sets the POLLHUP bit in revents and returns immediately, > regardless of the events we are querying for. > > There does not seem to be a solution to this with just poll() alone. > Using the TIOCPKT ioctl sounds promising, but doesn't help either, > as poll still detects the HUP condition. Are you sure? I couldn't observe this unless I also passed POLLOUT in events. poll(2) describes POLLHUP as only applying to writes. The behaviour of select() though seems to be that once the slave has been closed for the first time, reading the master fd screams EIO until the slave is opened again, so you have to fall back to periodically polling until the EIO goes away. I'm guessing poll() doesn't provide a reliable way to work around this either, hence this patch. > So apart from chickening out with some poll() timeout tricks, inotify > seems to be the way to go: > Each time poll() returns with the POLLHUP bit set, we disable this > file descriptor in the poll() array and rely on the inotify IN_OPEN > watch to fire on the slave end of the pseudoterminal. We then enable the > file descriptor again. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > term.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 3 deletions(-) > > diff --git a/term.c b/term.c > index b8a70fe2..7fbd98c6 100644 > --- a/term.c > +++ b/term.c > @@ -7,6 +7,7 @@ > #include <signal.h> > #include <pty.h> > #include <utmp.h> > +#include <sys/inotify.h> > > #include "kvm/read-write.h" > #include "kvm/term.h" > @@ -21,6 +22,8 @@ static struct termios orig_term; > > static int term_fds[TERM_MAX_DEVS][2]; > > +static int inotify_fd; > + > static pthread_t term_poll_thread; > > /* ctrl-a is used for escape */ > @@ -100,7 +103,7 @@ bool term_readable(int term) > > static void *term_poll_thread_loop(void *param) > { > - struct pollfd fds[TERM_MAX_DEVS]; > + struct pollfd fds[TERM_MAX_DEVS + 1]; > struct kvm *kvm = (struct kvm *) param; > int i; > > @@ -111,11 +114,42 @@ static void *term_poll_thread_loop(void *param) > fds[i].events = POLLIN; > fds[i].revents = 0; > } > + fds[i].fd = inotify_fd; > + fds[i].events = POLLIN; > + fds[i].revents = 0; > > while (1) { > + int i; > + > /* Poll with infinite timeout */ > - if(poll(fds, TERM_MAX_DEVS, -1) < 1) > + if(poll(fds, TERM_MAX_DEVS + 1, -1) < 1) > break; > + > + for (i = 0; i < TERM_MAX_DEVS; i++) { > + /* > + * Check for unconnected pseudoterminals. They will > + * make poll() return immediately, so we have to > + * disable those fds and rely on inotify to tell us > + * when the slave side gets opened. > + */ > + if (fds[i].revents == POLLHUP) Should this be & ? Or is POLLHUP always delivered by itself? > + fds[i].fd = ~fds[i].fd; > + } > + if (fds[TERM_MAX_DEVS].revents) { /* inotify fd */ > + struct inotify_event event; > + > + /* > + * Just enable all fds that we previously disabled, > + * still unconnected ones will be disabled again on > + * the next poll() call. > + */ > + for (i = 0; i < TERM_MAX_DEVS; i++) > + if (fds[i].fd < 0) > + fds[i].fd = ~fds[i].fd; > + /* Consume at least one inotify event. */ > + i = read(inotify_fd, &event, sizeof(event)); Are there raciness / event loss issues here? If we just toggle the fds on each open, then opening the slave again when it is already open will break things, no? (This is peephole review, so I may be missing some wider context.) Also, I'm not sure (actually, I strongly doubt) that anything guarantees that we see the EIO/POLLHUP for a hung-up slave before the inotify() notify notification that reopens it. Maybe I'm being too paranoid (as often the case). We could try to maintain a counter if we also track IN_CLOSE, but inotify queue overflows are still a potential problem. Even if we get this working, we're relying on a bunch of undocumented behaviour that could drift in future. I'd like all this to work ... but given that ptys don't appear well designed to solve this kind of problem, I wonder whether it's really worth trying to support them? Sockets OTOH are designed for this use case and support remote access at no extra cost. Do we need genuine terminal emulation for something? [...] Cheers ---Dave
diff --git a/term.c b/term.c index b8a70fe2..7fbd98c6 100644 --- a/term.c +++ b/term.c @@ -7,6 +7,7 @@ #include <signal.h> #include <pty.h> #include <utmp.h> +#include <sys/inotify.h> #include "kvm/read-write.h" #include "kvm/term.h" @@ -21,6 +22,8 @@ static struct termios orig_term; static int term_fds[TERM_MAX_DEVS][2]; +static int inotify_fd; + static pthread_t term_poll_thread; /* ctrl-a is used for escape */ @@ -100,7 +103,7 @@ bool term_readable(int term) static void *term_poll_thread_loop(void *param) { - struct pollfd fds[TERM_MAX_DEVS]; + struct pollfd fds[TERM_MAX_DEVS + 1]; struct kvm *kvm = (struct kvm *) param; int i; @@ -111,11 +114,42 @@ static void *term_poll_thread_loop(void *param) fds[i].events = POLLIN; fds[i].revents = 0; } + fds[i].fd = inotify_fd; + fds[i].events = POLLIN; + fds[i].revents = 0; while (1) { + int i; + /* Poll with infinite timeout */ - if(poll(fds, TERM_MAX_DEVS, -1) < 1) + if(poll(fds, TERM_MAX_DEVS + 1, -1) < 1) break; + + for (i = 0; i < TERM_MAX_DEVS; i++) { + /* + * Check for unconnected pseudoterminals. They will + * make poll() return immediately, so we have to + * disable those fds and rely on inotify to tell us + * when the slave side gets opened. + */ + if (fds[i].revents == POLLHUP) + fds[i].fd = ~fds[i].fd; + } + if (fds[TERM_MAX_DEVS].revents) { /* inotify fd */ + struct inotify_event event; + + /* + * Just enable all fds that we previously disabled, + * still unconnected ones will be disabled again on + * the next poll() call. + */ + for (i = 0; i < TERM_MAX_DEVS; i++) + if (fds[i].fd < 0) + fds[i].fd = ~fds[i].fd; + /* Consume at least one inotify event. */ + i = read(inotify_fd, &event, sizeof(event)); + } + kvm__arch_read_term(kvm); } @@ -154,7 +188,11 @@ static void term_set_tty(int term) close(slave); - pr_info("Assigned terminal %d to pty %s\n", term, new_pty); + pr_info("Assigned terminal %d to pty %s", term, new_pty); + + if (!inotify_fd) + inotify_fd = inotify_init(); + inotify_add_watch(inotify_fd, new_pty, IN_OPEN); term_fds[term][TERM_FD_IN] = term_fds[term][TERM_FD_OUT] = master; } @@ -194,6 +232,10 @@ static int term_init(struct kvm *kvm) term.c_lflag &= ~(ICANON | ECHO | ISIG); tcsetattr(STDIN_FILENO, TCSANOW, &term); + if (!inotify_fd) + inotify_fd = inotify_init(); + if (inotify_fd < 0) + die("Unable to initialise inotify\n"); /* Use our own blocking thread to read stdin, don't require a tick */ if(pthread_create(&term_poll_thread, NULL, term_poll_thread_loop,kvm))
Currently when kvmtool is creating a pseudoterminal (--tty x), the terminal thread will consume 100% of its CPU time as long as no slave is connected to the other end. This is due to the fact that poll() unconditonally sets the POLLHUP bit in revents and returns immediately, regardless of the events we are querying for. There does not seem to be a solution to this with just poll() alone. Using the TIOCPKT ioctl sounds promising, but doesn't help either, as poll still detects the HUP condition. So apart from chickening out with some poll() timeout tricks, inotify seems to be the way to go: Each time poll() returns with the POLLHUP bit set, we disable this file descriptor in the poll() array and rely on the inotify IN_OPEN watch to fire on the slave end of the pseudoterminal. We then enable the file descriptor again. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- term.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-)