Message-ID: <20150501162040.05c0cb42@notabene.brown>
Date:	Fri, 1 May 2015 16:20:40 +1000
From:	NeilBrown <neilb@...e.de>
To:	Peter Hurley <peter@...leysoftware.com>
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: [PATCH bisected regression] input_available_p() sometimes says 'no'
 when it should say 'yes'


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?

Thanks,
NeilBrown



------------------------------------------------------
#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;
 }
 
 /**

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists