Message-ID: <5543964C.9030606@hurleysoftware.com>
Date:	Fri, 01 May 2015 11:05:48 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	NeilBrown <neilb@...e.de>
CC:	Michael Matz <matz@...e.de>,
	Nic Percival <Nic.Percival@...rofocus.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bisected regression] input_available_p() sometimes says
 'no' when it should say 'yes'

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 <neilb@...e.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.Percival@...rofocus.com>
> Reported-by: Michael Matz <matz@...e.de>
> Fixes: f95499c3030f ("n_tty: Don't wait for buffer work in read() loop")
> Signed-off-by: NeilBrown <neilb@...e.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists