Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753958AbbEAPGM (ORCPT ); Fri, 1 May 2015 11:06:12 -0400 Received: from mail-qg0-f45.google.com ([209.85.192.45]:35959 "EHLO mail-qg0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753914AbbEAPGH (ORCPT ); Fri, 1 May 2015 11:06:07 -0400 Message-ID: <5543964C.9030606@hurleysoftware.com> Date: Fri, 01 May 2015 11:05:48 -0400 From: Peter Hurley User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: NeilBrown CC: Michael Matz , Nic Percival , Greg Kroah-Hartman , Jiri Slaby , linux-kernel@vger.kernel.org Subject: Re: [PATCH bisected regression] input_available_p() sometimes says 'no' when it should say 'yes' References: <20150501162040.05c0cb42@notabene.brown> In-Reply-To: <20150501162040.05c0cb42@notabene.brown> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9679 Lines: 383 Hi Neil, On 05/01/2015 02:20 AM, NeilBrown wrote: > > Hi Peter, > I recently had a report of a regression in 3.12. I bisected it down to your > patch > f95499c3030f ("n_tty: Don't wait for buffer work in read() loop") > > Sometimes a poll on a master-pty will report there is nothing to read after > the slave has written something. > As test program is below. > On a kernel prior to your commit, this program never reports > > Total bytes read is 0. PollRC=0 > > On a kernel subsequent to your commit, that message is produced quite often. > > This was found while working on a debugger. > > Following the test program is my proposed patch which allows the program to > run as it used to. It re-introduces the call to tty_flush_to_ldisc(), but > only if it appears that there is nothing to read. > > Do you think this is a suitable fix? Do you even agree that it is a real > bug? I don't think this a real bug, in the sense that pty i/o is not synchronous, in the same way that tty i/o is not synchronous. However, that said, if this is a regression (regression as in "it broke something that used to work", not regression as in "this new thing I'm writing doesn't behave the way I want it to" :) ) Help me understand the use-case here: are you using pty i/o to debug the debugger? Regards, Peter Hurley > ------------------------------------------------------ > #define _XOPEN_SOURCE > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > > > #define USEPTY > #define COUNT_MAX (5000000) > #define MY_BREAKPOINT { asm("int $3"); } > #define PTRACE_IGNORED (void *)0 > > /* > ** Open s pseudo-tty pair. > ** > ** Return the master fd, set spty to the slave fd. > */ > int > my_openpt(int *spty) > { > int mfd = posix_openpt(O_RDWR | O_NOCTTY); > char *slavedev; > int sfd=-1; > if(mfd == -1) return -1; > if(grantpt(mfd) == -1) return -1; > if(unlockpt(mfd) == -1) return -1; > > slavedev = (char *)ptsname(mfd); > > if((sfd = open(slavedev, O_RDWR | O_NOCTTY)) == -1) > { > close(mfd); > return -1; > } > if(spty != NULL) > { > *spty = sfd; > } > return mfd; > } > > > /* > ** Read from the provided file descriptor if poll says there's > ** anything there.. > */ > int > DoPollRead(int mpty) > { > struct pollfd fds; > int pollrc; > ssize_t bread=0, totread=0; > char readbuf[101]; > > /* > ** Set up the poll. > */ > fds.fd = mpty; > fds.events = POLLIN | POLLRDNORM | POLLRDBAND | POLLPRI; > > /* > ** poll for any output. > */ > > while((pollrc = poll(&fds, 1, 0)) == 1) > { > if(fds.revents & POLLIN) > { > bread = read( mpty, readbuf, 100 ); > totread += bread; > if(bread > 0) > { > //printf("Read %d bytes.\n", (int)bread); > readbuf[bread] = '\0'; > //printf("\t%s", readbuf); > } else > { > //puts("Nothing read.\n"); > } > } else if (fds.revents & (POLLHUP | POLLERR | POLLNVAL)) { > printf ("hangup/error/invalid on poll\n"); > return totread; > } else { printf("No POLLIN, revents=%d\n", fds.revents); }; > } > > /* > ** This sometimes happens - we're expecting input on the pty, > ** but nothing is there. > */ > if(totread == 0) > printf("Total bytes read is 0. PollRC=%d\n", pollrc); > > return totread; > } > > static > void writeall (int fd, const char *buf, size_t count) > { > while (count) > { > ssize_t r = write (fd, buf, count); > if (r == 0) > break; > if (r < 0 && errno == EINTR) > continue; > if (r < 0) > exit (2); > count -= r; > buf += r; > } > } > > int > thechild(void) > { > unsigned int i; > > writeall (1, "debuggee starts\n", strlen ("debuggee starts\n")); > > for(i=0 ; i { > char buf[100]; > sprintf(buf, "This is the debuggee. Count is %d\n", i); > writeall (1, buf, strlen (buf)); > > MY_BREAKPOINT > } > > writeall (1, "debuggee finishing now.\n", strlen ("debuggee finishing now.\n")); > exit (0); > } > > int > main() > { > int rv, status, i=0; > pid_t pid; > int sfd = -1; > int mfd; > #ifdef USEPTY > mfd = my_openpt(&sfd); /* Get a pseudo-tty pair. */ > if(mfd < 0) > { > fprintf(stderr, "Failed to create pty\n"); > return(1); > } > #else > int pipefd[2]; > if (pipe (pipefd) < 0) > { > perror ("pipe"); > return 1; > } > mfd = pipefd[0]; > sfd = pipefd[1]; > #endif > > /* > ** Create a child process. > */ > pid = fork(); > switch(pid) > { > case -1: /* failed fork */ > return -1; > case 0: /* child process */ > > close (mfd); > /* > ** Close stdout, use the slave pty for output. > */ > dup2(sfd, STDOUT_FILENO); > > > /* > ** Set 'TRACEME' so this child process can be traced by the > ** parent process. > */ > ptrace(PTRACE_TRACEME, > PTRACE_IGNORED, PTRACE_IGNORED, PTRACE_IGNORED); > thechild (); > break; > > default: /* parent process drops out of switch */ > close (sfd); > break; > } > > /* > ** Wait for the debuggee to hit the traceme. > ** When we see this, immediately send a PTRACE_CONT to kick off > ** the debuggee.. > */ > rv = waitpid(pid, &status, 0); > if(WIFSTOPPED(status)) > { > printf("Debuggee initial stop. Sending PTRACE_CONT..\n"); > ptrace(PTRACE_CONT, pid, PTRACE_IGNORED,PTRACE_IGNORED); > } > else > { > printf("Failure of debuggee to hit initial 'traceme'\n"); > return 99; > } > > /* > ** Loop around, waiting for the debuggee to hit its own breakpoint. > ** Look for output on the pty. Each time around we should see a line > ** from the debuggee. > **/ > while(1) > { > int pollrc, stopped, exited; > > //printf("Debugger doing a waitpid on debuggee..(%d)\n", i); > i++; > > rv = waitpid(pid, &status, 0); > stopped=0; > exited=0; > > /* > ** Ok, the waitpid has returned. What's happened to the debuggee? > ** Note we do recheck rv and drop out later if it's -1. > */ > if(rv == -1) > { > printf("Debuggee process has died?. Checking for output.\n"); > } > else if(WIFSTOPPED(status)) > { > //printf("Debuggee process has stopped. Checking for output.\n"); > stopped=1; > } > else if(!WIFEXITED(status)) > { > printf("Debuggee has exited. Checking for output.\n"); > exited=1; > } > else > { > printf("WEXITSTATUS is %d\n", WEXITSTATUS(status)); > exited=1; > } > > /* > ** See if there's anything to read. If so display it. > ** There always should be, the debuggee writes output on each > ** iteration and fflush's it. > */ > (void) DoPollRead(mfd); > > /* > ** If the debuggee has stopped tell it to continue. If it's > ** exited detach from it. > */ > if(stopped) > { > //puts("Sending PTRACE_CONT"); > ptrace(PTRACE_CONT, pid, PTRACE_IGNORED,PTRACE_IGNORED); > } else if(exited) > { > printf("Debuggee exited, detaching\n"); > ptrace(PTRACE_DETACH, pid, PTRACE_IGNORED,PTRACE_IGNORED); > return 0; > } else if(rv == -1) > { > printf("Debuggee died? Leaving.\n"); > return 98; > } > } > } > ------------------------------------------------------ > > > > From: NeilBrown > Subject: [PATCH] n_tty: Sometimes wait for buffer work in read() loop > > Since commit > f95499c3030f ("n_tty: Don't wait for buffer work in read() loop") > > it as been possible for poll to report that there is no data to read > on a master-pty even if a write to the slave has actually completed. > > That patch removes a 'wait' when the wait isn't really necessary. > Unfortunately it also removed it in the case when it *is* necessary. > If the simple tests show that there is nothing to read, we really need > to flush the work queue in case there is something ready but which > hasn't arrived yet. > > This patch restores the wait, but only if simple tests suggest there > is nothing ready. > > Reported-by: Nic Percival > Reported-by: Michael Matz > Fixes: f95499c3030f ("n_tty: Don't wait for buffer work in read() loop") > Signed-off-by: NeilBrown > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index cf6e0f2e1331..9884091819b6 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -1942,11 +1942,18 @@ static inline int input_available_p(struct tty_struct *tty, int poll) > { > struct n_tty_data *ldata = tty->disc_data; > int amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1; > - > - if (ldata->icanon && !L_EXTPROC(tty)) > - return ldata->canon_head != ldata->read_tail; > - else > - return ldata->commit_head - ldata->read_tail >= amt; > + int i; > + int ret = 0; > + > + for (i = 0; !ret && i < 2; i++) { > + if (i) > + tty_flush_to_ldisc(tty); > + if (ldata->icanon && !L_EXTPROC(tty)) > + ret = (ldata->canon_head != ldata->read_tail); > + else > + ret = (ldata->commit_head - ldata->read_tail >= amt); > + } > + return ret; > } > > /** > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/