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<unistd.h>
> #include<stdlib.h>
> #include<stdio.h>
> #include<stdlib.h>
> #include<string.h>
> #include<errno.h>
> #include<sys/wait.h>
> #include<sys/types.h>
> #include<sys/ptrace.h>
> #include<asm/ptrace.h>
> #include<fcntl.h>
> #include<sys/poll.h>
>
>
> #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<COUNT_MAX ; 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 <ne...@suse.de>
> 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 <nic.perci...@microfocus.com>
> Reported-by: Michael Matz <m...@suse.de>
> Fixes: f95499c3030f ("n_tty: Don't wait for buffer work in read() loop")
> Signed-off-by: NeilBrown <ne...@suse.de>
>
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
