[PATCH tty-next 0/7] n_tty cleanup + trace additions

[PATCH tty-next 0/7] n_tty cleanup + trace additions

[Peter Hurley]

Greg,

This patchset contains no bug fixes only misc cleanup for the
N_TTY line discipline.

Patches 1-4 cleans up code duplication remnants from 3.12's changes.
Patch 5 simplifies the snarled logic of how much data is considered
'input available'. I was getting tired of hand checking that logic
every time I touched minimum_to_wake.
Patch 6 reduces unnecessary wake ups.
Patch 7 extends the trace usage to aid in identifying and fixing
input flow-control bugs (this was used to fix the readline() bug).
I also expect to use it to reduce input processing restarts. It's
use is disabled by default and only enabled with the file-local
define N_TTY_TRACE (similar to TTY_DEBUG_HANGUP in tty_io.c).

Regards,

Peter Hurley (7):
  n_tty: Merge .receive_buf() flavors
  n_tty: Un-inline slow-path n_tty_receive_char()
  n_tty: Un-inline slow-path n_tty_receive_char_closing()
  n_tty: Refactor PARMRK doubling checks
  n_tty: Refactor input_available_p() by call site
  n_tty: Only perform wakeups for waiters
  n_tty: trace input/read flow control

 drivers/tty/n_tty.c | 159 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 97 insertions(+), 62 deletions(-)

-- 
1.8.1.2

[PATCH tty-next 1/7] n_tty: Merge .receive_buf() flavors

[Peter Hurley]

N_TTY's direct and flow-controlled flavors of the .receive_buf()
method are nearly identical; fold together.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 43 ++++++++++++++++---------------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 5f1c5f3..3eb54c7 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1675,32 +1675,9 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	}
 }
 
-static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
-			      char *fp, int count)
-{
-	int room, n;
-
-	down_read(&tty->termios_rwsem);
-
-	while (1) {
-		room = receive_room(tty);
-		n = min(count, room);
-		if (!n)
-			break;
-		__receive_buf(tty, cp, fp, n);
-		cp += n;
-		if (fp)
-			fp += n;
-		count -= n;
-	}
-
-	tty->receive_room = room;
-	n_tty_check_throttle(tty);
-	up_read(&tty->termios_rwsem);
-}
-
-static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
-			      char *fp, int count)
+static int
+n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
+			 char *fp, int count, int flow)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 	int room, n, rcvd = 0;
@@ -1711,7 +1688,7 @@ static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
 		room = receive_room(tty);
 		n = min(count, room);
 		if (!n) {
-			if (!room)
+			if (flow && !room)
 				ldata->no_room = 1;
 			break;
 		}
@@ -1730,6 +1707,18 @@ static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
 	return rcvd;
 }
 
+static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
+			      char *fp, int count)
+{
+	n_tty_receive_buf_common(tty, cp, fp, count, 0);
+}
+
+static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
+			      char *fp, int count)
+{
+	return n_tty_receive_buf_common(tty, cp, fp, count, 1);
+}
+
 int is_ignored(int sig)
 {
 	return (sigismember(&current->blocked, sig) ||
-- 
1.8.1.2

[PATCH tty-next 2/7] n_tty: Un-inline slow-path n_tty_receive_char()

[Peter Hurley]

Commit e60d27c4d8b33ba20896b76b6558f061bc6460ff,
n_tty: Factor LNEXT processing from per-char i/o path,
mistakenly inlined the non-inline alias, n_tty_receive_char(),
for the inline function, n_tty_receive_char_inline().

As n_tty_receive_char() is intended for slow-path char
processing only, un-inline it.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 3eb54c7..3808d3d 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1418,7 +1418,7 @@ n_tty_receive_char_inline(struct tty_struct *tty, unsigned char c)
 	put_tty_queue(c, ldata);
 }
 
-static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
+static void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 {
 	n_tty_receive_char_inline(tty, c);
 }
-- 
1.8.1.2

[PATCH tty-next 3/7] n_tty: Un-inline slow-path n_tty_receive_char_closing()

[Peter Hurley]

Although n_tty_receive_char_closing() only has one call-site,
let the compiler inline instead.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 3808d3d..61edeaf 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1443,8 +1443,7 @@ n_tty_receive_char_fast(struct tty_struct *tty, unsigned char c)
 	put_tty_queue(c, ldata);
 }
 
-static inline void
-n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c)
+static void n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c)
 {
 	if (I_ISTRIP(tty))
 		c &= 0x7f;
-- 
1.8.1.2

[PATCH tty-next 4/7] n_tty: Refactor PARMRK doubling checks

[Peter Hurley]

Perform PARMRK doubling checks explicitly; remove ternary idiom
and local variable.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 61edeaf..9298b68 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1258,7 +1258,6 @@ static int
 n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
 {
 	struct n_tty_data *ldata = tty->disc_data;
-	int parmrk;
 
 	if (I_IXON(tty)) {
 		if (c == START_CHAR(tty)) {
@@ -1343,8 +1342,6 @@ n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
 		}
 		if ((c == EOL_CHAR(tty)) ||
 		    (c == EOL2_CHAR(tty) && L_IEXTEN(tty))) {
-			parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty))
-				 ? 1 : 0;
 			/*
 			 * XXX are EOL_CHAR and EOL2_CHAR echoed?!?
 			 */
@@ -1359,7 +1356,7 @@ n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
 			 * XXX does PARMRK doubling happen for
 			 * EOL_CHAR and EOL2_CHAR?
 			 */
-			if (parmrk)
+			if (c == (unsigned char) '\377' && I_PARMRK(tty))
 				put_tty_queue(c, ldata);
 
 handle_newline:
@@ -1373,7 +1370,6 @@ handle_newline:
 		}
 	}
 
-	parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
 	if (L_ECHO(tty)) {
 		finish_erasing(ldata);
 		if (c == '\n')
@@ -1387,7 +1383,8 @@ handle_newline:
 		commit_echoes(tty);
 	}
 
-	if (parmrk)
+	/* PARMRK doubling check */
+	if (c == (unsigned char) '\377' && I_PARMRK(tty))
 		put_tty_queue(c, ldata);
 
 	put_tty_queue(c, ldata);
@@ -1398,7 +1395,6 @@ static inline void
 n_tty_receive_char_inline(struct tty_struct *tty, unsigned char c)
 {
 	struct n_tty_data *ldata = tty->disc_data;
-	int parmrk;
 
 	if (tty->stopped && !tty->flow_stopped && I_IXON(tty) && I_IXANY(tty)) {
 		start_tty(tty);
@@ -1412,8 +1408,8 @@ n_tty_receive_char_inline(struct tty_struct *tty, unsigned char c)
 		echo_char(c, tty);
 		commit_echoes(tty);
 	}
-	parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
-	if (parmrk)
+	/* PARMRK doubling check */
+	if (c == (unsigned char) '\377' && I_PARMRK(tty))
 		put_tty_queue(c, ldata);
 	put_tty_queue(c, ldata);
 }
-- 
1.8.1.2

[PATCH tty-next 5/7] n_tty: Refactor input_available_p() by call site

[Peter Hurley]

Distinguish if caller is n_tty_poll() or n_tty_read(), and
set the read/wakeup threshold accordingly.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 9298b68..8f2356e 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1878,14 +1878,15 @@ static int n_tty_open(struct tty_struct *tty)
 	return 0;
 }
 
-static inline int input_available_p(struct tty_struct *tty, int amt)
+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) : 1;
 
 	if (ldata->icanon && !L_EXTPROC(tty)) {
 		if (ldata->canon_head != ldata->read_tail)
 			return 1;
-	} else if (read_cnt(ldata) >= (amt ? amt : 1))
+	} else if (read_cnt(ldata) >= amt)
 		return 1;
 
 	return 0;
@@ -2384,7 +2385,7 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 
 	poll_wait(file, &tty->read_wait, wait);
 	poll_wait(file, &tty->write_wait, wait);
-	if (input_available_p(tty, TIME_CHAR(tty) ? 0 : MIN_CHAR(tty)))
+	if (input_available_p(tty, 1))
 		mask |= POLLIN | POLLRDNORM;
 	else if (ldata->icanon)
 		n_tty_set_room(tty);
-- 
1.8.1.2

Re: [PATCH tty-next 5/7] n_tty: Refactor input_available_p() by call site

[One Thousand Gnomes]

On Fri, 22 Nov 2013 10:59:23 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> Distinguish if caller is n_tty_poll() or n_tty_read(), and
> set the read/wakeup threshold accordingly.

That looks very wrong to me. The poll() and read() wakeup should always
match. In addition if MIN_CHAR is set without a timeout then it should not
wake up until MIN_CHAR characters are present. The MIN_CHAR feature is
used by various (these days obscure) protocols to optimise block transfer
rates.

Changing this is also a userspace visible API change.

So NAK.

The underlying intention of the system (and SYS5.4) is that you can do

	MIN_CHAR = 64

	alarm(some_timeout)
	while(1) {
		poll();
		events
		read(chunksized blocks)
	}

while it's hardly as critical with modern fast hardware its still the API
and necessary to avoid SWS type behaviours in the queue turning it from
block/syscall to byte I/O.

Alan

Re: [PATCH tty-next 5/7] n_tty: Refactor input_available_p() by call site

[One Thousand Gnomes]

On Fri, 22 Nov 2013 10:59:23 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> Distinguish if caller is n_tty_poll() or n_tty_read(), and
> set the read/wakeup threshold accordingly.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

Doh ignore previous.. yes that patch is right.  I should pay attention to
0/7 8)

Re: [PATCH tty-next 5/7] n_tty: Refactor input_available_p() by call site

[Peter Hurley]

On 11/23/2013 07:26 PM, One Thousand Gnomes wrote:
> On Fri, 22 Nov 2013 10:59:23 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
>
>> Distinguish if caller is n_tty_poll() or n_tty_read(), and
>> set the read/wakeup threshold accordingly.
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>
> Doh ignore previous.. yes that patch is right.  I should pay attention to
> 0/7 8)

Maybe I should add more to the commit message from the cover letter
description ?

Re: [PATCH tty-next 5/7] n_tty: Refactor input_available_p() by call site

[One Thousand Gnomes]

On Sat, 23 Nov 2013 21:01:56 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> On 11/23/2013 07:26 PM, One Thousand Gnomes wrote:
> > On Fri, 22 Nov 2013 10:59:23 -0500
> > Peter Hurley <peter@hurleysoftware.com> wrote:
> >
> >> Distinguish if caller is n_tty_poll() or n_tty_read(), and
> >> set the read/wakeup threshold accordingly.
> >>
> >> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> >
> > Doh ignore previous.. yes that patch is right.  I should pay attention to
> > 0/7 8)
> 
> Maybe I should add more to the commit message from the cover letter
> description ?

Maybe I should just read the code more carefully 8).

Alan

[PATCH tty-next 6/7] n_tty: Only perform wakeups for waiters

[Peter Hurley]

Only wakeup the _waiting_ reader, polls and/or writer(s).

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 8f2356e..aae28a6 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -275,7 +275,8 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 			return;
 		n_tty_set_room(tty);
 		n_tty_write_wakeup(tty->link);
-		wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
+		if (waitqueue_active(&tty->link->write_wait))
+			wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
 		return;
 	}
 
@@ -350,7 +351,8 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
 	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (tty->link->packet) {
 		tty->ctrl_status |= TIOCPKT_FLUSHREAD;
-		wake_up_interruptible(&tty->link->read_wait);
+		if (waitqueue_active(&tty->link->read_wait))
+			wake_up_interruptible(&tty->link->read_wait);
 	}
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 }
@@ -1156,7 +1158,8 @@ static void n_tty_receive_break(struct tty_struct *tty)
 		put_tty_queue('\0', ldata);
 	}
 	put_tty_queue('\0', ldata);
-	wake_up_interruptible(&tty->read_wait);
+	if (waitqueue_active(&tty->read_wait))
+		wake_up_interruptible(&tty->read_wait);
 }
 
 /**
@@ -1214,7 +1217,8 @@ static void n_tty_receive_parity_error(struct tty_struct *tty, unsigned char c)
 		put_tty_queue('\0', ldata);
 	else
 		put_tty_queue(c, ldata);
-	wake_up_interruptible(&tty->read_wait);
+	if (waitqueue_active(&tty->read_wait))
+		wake_up_interruptible(&tty->read_wait);
 }
 
 static void
@@ -1808,8 +1812,10 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 		start_tty(tty);
 
 	/* The termios change make the tty ready for I/O */
-	wake_up_interruptible(&tty->write_wait);
-	wake_up_interruptible(&tty->read_wait);
+	if (waitqueue_active(&tty->write_wait))
+		wake_up_interruptible(&tty->write_wait);
+	if (waitqueue_active(&tty->read_wait))
+		wake_up_interruptible(&tty->read_wait);
 }
 
 /**
-- 
1.8.1.2

Re: [PATCH tty-next 6/7] n_tty: Only perform wakeups for waiters

[One Thousand Gnomes]

On Fri, 22 Nov 2013 10:59:24 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> Only wakeup the _waiting_ reader, polls and/or writer(s).
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/n_tty.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 8f2356e..aae28a6 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -275,7 +275,8 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
>  			return;
>  		n_tty_set_room(tty);
>  		n_tty_write_wakeup(tty->link);
> -		wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
> +		if (waitqueue_active(&tty->link->write_wait))
> +			wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);

Does this actually microbenchmark faster ?

Alan

Re: [PATCH tty-next 6/7] n_tty: Only perform wakeups for waiters

[Peter Hurley]

On 11/23/2013 07:23 PM, One Thousand Gnomes wrote:
> On Fri, 22 Nov 2013 10:59:24 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
>
>> Only wakeup the _waiting_ reader, polls and/or writer(s).
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>   drivers/tty/n_tty.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
>> index 8f2356e..aae28a6 100644
>> --- a/drivers/tty/n_tty.c
>> +++ b/drivers/tty/n_tty.c
>> @@ -275,7 +275,8 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
>>   			return;
>>   		n_tty_set_room(tty);
>>   		n_tty_write_wakeup(tty->link);
>> -		wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
>> +		if (waitqueue_active(&tty->link->write_wait))
>> +			wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
>
> Does this actually microbenchmark faster ?

Getting on and off the write_wait queue is actually pretty expensive for
the "other" pty (the writer), and the unnecessary wakeup from the reader
doesn't help.

The other chunks are gratuitous.

Regards,
Peter Hurley

PS - This came up because there is some worst-case behavior that I'm looking
into fixing.  When the userspace reader is very far behind (say because it's
reading char-by-char), it doesn't make sense to keep restarting the input
processing worker.

[PATCH tty-next 7/7] n_tty: trace input/read flow control

[Peter Hurley]

Instrument .receive_buf() and read() paths with trace_printk's
to aid in debugging flow control changes.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 76 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 60 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index aae28a6..68fa3a4 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -83,9 +83,14 @@
 
 #undef N_TTY_TRACE
 #ifdef N_TTY_TRACE
-# define n_tty_trace(f, args...)	trace_printk(f, ##args)
+# define n_tty_trace(tty, f, args...)					\
+	do {								\
+		char buf[64];						\
+		trace_printk("[%s] " f, tty_name(tty, buf), ##args);	\
+	} while (0)
 #else
-# define n_tty_trace(f, args...)
+# define n_tty_trace(tty, f, args...)	\
+	do { } while (0)
 #endif
 
 struct n_tty_data {
@@ -179,8 +184,13 @@ static int receive_room(struct tty_struct *tty)
 	 * that erase characters will be handled.  Other excess
 	 * characters will be beeped.
 	 */
-	if (left <= 0)
+	if (left <= 0) {
 		left = ldata->icanon && ldata->canon_head == ldata->read_tail;
+		if (left) {
+			n_tty_trace(tty, "overflow, head:%zu tail:%zu\n",
+				    ldata->read_head, ldata->read_tail);
+		}
+	}
 
 	return left;
 }
@@ -200,11 +210,14 @@ static int receive_room(struct tty_struct *tty)
 static void n_tty_set_room(struct tty_struct *tty)
 {
 	struct n_tty_data *ldata = tty->disc_data;
+	int avail;
 
 	/* Did this open up the receive buffer? We may need to flip */
-	if (unlikely(ldata->no_room) && receive_room(tty)) {
+	if (unlikely(ldata->no_room) && (avail = receive_room(tty))) {
 		ldata->no_room = 0;
 
+		n_tty_trace(tty, "restart worker (avail = %d)\n", avail);
+
 		WARN_RATELIMIT(tty->port->itty == NULL,
 				"scheduling with invalid itty\n");
 		/* see if ldisc has been killed - if so, this means that
@@ -240,6 +253,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
+	n_tty_trace(tty, "\n");
+
 	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
 		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }
@@ -254,13 +269,16 @@ static void n_tty_check_throttle(struct tty_struct *tty)
 	 * canonical mode and don't have a newline yet!
 	 */
 	while (1) {
-		int throttled;
+		int throttled, avail;
 		tty_set_flow_change(tty, TTY_THROTTLE_SAFE);
-		if (receive_room(tty) >= TTY_THRESHOLD_THROTTLE)
+		avail = receive_room(tty);
+		if (avail >= TTY_THRESHOLD_THROTTLE)
 			break;
 		throttled = tty_throttle_safe(tty);
-		if (!throttled)
+		if (!throttled) {
+			n_tty_trace(tty, "throttled (avail = %d)/n", avail);
 			break;
+		}
 	}
 	__tty_set_flow_change(tty, 0);
 }
@@ -289,16 +307,19 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 	 */
 
 	while (1) {
-		int unthrottled;
+		int unthrottled, used;
 		tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
-		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
+		used = chars_in_buffer(tty);
+		if (used > TTY_THRESHOLD_UNTHROTTLE)
 			break;
 		if (!tty->count)
 			break;
 		n_tty_set_room(tty);
 		unthrottled = tty_unthrottle_safe(tty);
-		if (!unthrottled)
+		if (!unthrottled) {
+			n_tty_trace(tty, "unthrottled (used = %d)\n", used);
 			break;
+		}
 	}
 	__tty_set_flow_change(tty, 0);
 }
@@ -374,6 +395,9 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
 static void n_tty_flush_buffer(struct tty_struct *tty)
 {
 	down_write(&tty->termios_rwsem);
+
+	n_tty_trace(tty, "\n");
+
 	reset_buffer_flags(tty->disc_data);
 	n_tty_set_room(tty);
 
@@ -770,6 +794,10 @@ static size_t __process_echoes(struct tty_struct *tty)
 	 * of echo overrun before the next commit), then discard enough
 	 * data at the tail to prevent a subsequent overrun */
 	while (ldata->echo_commit - tail >= ECHO_DISCARD_WATERMARK) {
+
+		n_tty_trace(tty, "discard echoes (%zu - %zu > %d)\n",
+			    ldata->echo_commit, tail, ECHO_DISCARD_WATERMARK);
+
 		if (echo_buf(ldata, tail) == ECHO_OP_START) {
 			if (echo_buf(ldata, tail + 1) == ECHO_OP_ERASE_TAB)
 				tail += 3;
@@ -1367,6 +1395,7 @@ handle_newline:
 			set_bit(ldata->read_head & (N_TTY_BUF_SIZE - 1), ldata->read_flags);
 			put_tty_queue(c, ldata);
 			ldata->canon_head = ldata->read_head;
+			n_tty_trace(tty, "canon_head:%zu\n", ldata->canon_head);
 			kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 			if (waitqueue_active(&tty->read_wait))
 				wake_up_interruptible(&tty->read_wait);
@@ -1668,6 +1697,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 
 	if ((!ldata->icanon && (read_cnt(ldata) >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
+		n_tty_trace(tty, "reader wakeup, count:%zd\n", read_cnt(ldata));
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		if (waitqueue_active(&tty->read_wait))
 			wake_up_interruptible(&tty->read_wait);
@@ -1687,6 +1717,8 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
 		room = receive_room(tty);
 		n = min(count, room);
 		if (!n) {
+			if (!room)
+				n_tty_trace(tty, "buffer full\n");
 			if (flow && !room)
 				ldata->no_room = 1;
 			break;
@@ -1703,6 +1735,8 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
 	n_tty_check_throttle(tty);
 	up_read(&tty->termios_rwsem);
 
+	n_tty_trace(tty, "rcvd:%d\n", rcvd);
+
 	return rcvd;
 }
 
@@ -1811,6 +1845,8 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 	if (!I_IXON(tty) && old && (old->c_iflag & IXON) && !tty->flow_stopped)
 		start_tty(tty);
 
+	n_tty_trace(tty, "canon:%d\n", ldata->icanon);
+
 	/* The termios change make the tty ready for I/O */
 	if (waitqueue_active(&tty->write_wait))
 		wake_up_interruptible(&tty->write_wait);
@@ -1890,10 +1926,14 @@ static inline int input_available_p(struct tty_struct *tty, int poll)
 	int amt = poll && !TIME_CHAR(tty) ? MIN_CHAR(tty) : 1;
 
 	if (ldata->icanon && !L_EXTPROC(tty)) {
-		if (ldata->canon_head != ldata->read_tail)
+		if (ldata->canon_head != ldata->read_tail) {
+			n_tty_trace(tty, "line avail\n");
 			return 1;
-	} else if (read_cnt(ldata) >= amt)
+		}
+	} else if (read_cnt(ldata) >= amt) {
+		n_tty_trace(tty, "avail:%zu amt:%d\n", read_cnt(ldata), amt);
 		return 1;
+	}
 
 	return 0;
 }
@@ -1984,8 +2024,8 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1);
 	size = min_t(size_t, tail + n, N_TTY_BUF_SIZE);
 
-	n_tty_trace("%s: nr:%zu tail:%zu n:%zu size:%zu\n",
-		    __func__, *nr, tail, n, size);
+	n_tty_trace(tty, "nr:%zu tail:%zu n:%zu size:%zu\n",
+		    *nr, tail, n, size);
 
 	eol = find_next_bit(ldata->read_flags, size, tail);
 	more = n - (size - tail);
@@ -2009,8 +2049,8 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 		eof_push = !n && ldata->read_tail != ldata->line_start;
 	}
 
-	n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
-		    __func__, eol, found, n, c, size, more);
+	n_tty_trace(tty, "eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
+		    eol, found, n, c, size, more);
 
 	if (n > size) {
 		ret = copy_to_user(*b, read_buf_addr(ldata, tail), size);
@@ -2202,6 +2242,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 				n_tty_set_room(tty);
 				up_read(&tty->termios_rwsem);
 
+				n_tty_trace(tty, "blocking\n");
+
 				timeout = schedule_timeout(timeout);
 
 				down_read(&tty->termios_rwsem);
@@ -2259,6 +2301,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 	if (b - buf)
 		retval = b - buf;
 
+	n_tty_trace(tty, "ret:%zd\n", retval);
+
 	return retval;
 }
 
-- 
1.8.1.2

Re: [PATCH tty-next 7/7] n_tty: trace input/read flow control

[One Thousand Gnomes]

On Fri, 22 Nov 2013 10:59:25 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> Instrument .receive_buf() and read() paths with trace_printk's
> to aid in debugging flow control changes.

tty devices have a device, we have a dev_dbg() layer. The old tty trace
predates this but there isn't really any excuse for not using it now that
I can see ?

Alan

Re: [PATCH tty-next 7/7] n_tty: trace input/read flow control

[Peter Hurley]

On 11/23/2013 07:25 PM, One Thousand Gnomes wrote:
> On Fri, 22 Nov 2013 10:59:25 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
>
>> Instrument .receive_buf() and read() paths with trace_printk's
>> to aid in debugging flow control changes.
>
> tty devices have a device, we have a dev_dbg() layer. The old tty trace
> predates this but there isn't really any excuse for not using it now that
> I can see ?

I was using the ftrace facility because it has significantly less performance
impact than printk (which was an important factor while debugging flow control
problems).

That said, I could further macro-ize n_tty_trace() with selectable facility
(which would be useful when debugging problems that end in cpu death).

Regards,
Peter Hurley

Re: [PATCH tty-next 7/7] n_tty: trace input/read flow control

[Peter Hurley]

On 11/23/2013 09:38 PM, Peter Hurley wrote:
> On 11/23/2013 07:25 PM, One Thousand Gnomes wrote:
>> On Fri, 22 Nov 2013 10:59:25 -0500
>> Peter Hurley <peter@hurleysoftware.com> wrote:
>>
>>> Instrument .receive_buf() and read() paths with trace_printk's
>>> to aid in debugging flow control changes.
>>
>> tty devices have a device, we have a dev_dbg() layer. The old tty trace
>> predates this but there isn't really any excuse for not using it now that
>> I can see ?
>
> I was using the ftrace facility because it has significantly less performance
> impact than printk (which was an important factor while debugging flow control
> problems).
>
> That said, I could further macro-ize n_tty_trace() with selectable facility
> (which would be useful when debugging problems that end in cpu death).

Greg,

I don't really mind if you don't want to take this patch; it is useful but
it does clutter up the code.

Regards,
Peter Hurley

[PATCH v2 tty-next 0/8] n_tty cleanup + trace additions

[Peter Hurley]

Greg,

This patchset is a v2 respin which contains no bug fixes, only
misc cleanup for the N_TTY line discipline and significant
additions to debug tracing.

As I mentioned in a previous email, I can hold on to patches 7 and 8
and keep them out-of-tree if you'd rather not have instrumented code
in the tty layer.

v2:
- Patches 1-6 are unchanged from v1.
- Patch 7 changes: make n_tty_trace() configurable to output via
  pr_xxxx() or dev_dbg() rather than trace_printk() per Alan's comments
- Patch 8 adds echo buffer indices tracing (separately selectable)

v1:
Patches 1-4 cleans up code duplication remnants from 3.12's changes.
Patch 5 simplifies the snarled logic of how much data is considered
'input available'. I was getting tired of hand checking that logic
every time I touched minimum_to_wake.
Patch 6 reduces unnecessary wake ups.

Patch 7 extends the trace usage to aid in identifying and fixing
input flow-control bugs (this was used to fix the readline() bug).
I also expect to use it to reduce input processing restarts. It's
use is disabled by default and only enabled with the file-local
define N_TTY_TRACE (similar to TTY_DEBUG_HANGUP in tty_io.c).

Regards,

Peter Hurley (8):
  n_tty: Merge .receive_buf() flavors
  n_tty: Un-inline slow-path n_tty_receive_char()
  n_tty: Un-inline slow-path n_tty_receive_char_closing()
  n_tty: Refactor PARMRK doubling checks
  n_tty: Refactor input_available_p() by call site
  n_tty: Only perform wakeups for waiters
  n_tty: Extend debug tracing
  n_tty: Trace echo output with separate trace configuration

 drivers/tty/n_tty.c | 174 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 112 insertions(+), 62 deletions(-)

-- 
1.8.1.2

[PATCH v2 tty-next 1/8] n_tty: Merge .receive_buf() flavors

[Peter Hurley]

N_TTY's direct and flow-controlled flavors of the .receive_buf()
method are nearly identical; fold together.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 43 ++++++++++++++++---------------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d4d63b0..446ccfd 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1677,32 +1677,9 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	}
 }
 
-static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
-			      char *fp, int count)
-{
-	int room, n;
-
-	down_read(&tty->termios_rwsem);
-
-	while (1) {
-		room = receive_room(tty);
-		n = min(count, room);
-		if (!n)
-			break;
-		__receive_buf(tty, cp, fp, n);
-		cp += n;
-		if (fp)
-			fp += n;
-		count -= n;
-	}
-
-	tty->receive_room = room;
-	n_tty_check_throttle(tty);
-	up_read(&tty->termios_rwsem);
-}
-
-static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
-			      char *fp, int count)
+static int
+n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
+			 char *fp, int count, int flow)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 	int room, n, rcvd = 0;
@@ -1713,7 +1690,7 @@ static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
 		room = receive_room(tty);
 		n = min(count, room);
 		if (!n) {
-			if (!room)
+			if (flow && !room)
 				ldata->no_room = 1;
 			break;
 		}
@@ -1732,6 +1709,18 @@ static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
 	return rcvd;
 }
 
+static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
+			      char *fp, int count)
+{
+	n_tty_receive_buf_common(tty, cp, fp, count, 0);
+}
+
+static int n_tty_receive_buf2(struct tty_struct *tty, const unsigned char *cp,
+			      char *fp, int count)
+{
+	return n_tty_receive_buf_common(tty, cp, fp, count, 1);
+}
+
 int is_ignored(int sig)
 {
 	return (sigismember(&current->blocked, sig) ||
-- 
1.8.1.2

[PATCH v2 tty-next 2/8] n_tty: Un-inline slow-path n_tty_receive_char()

[Peter Hurley]

Commit e60d27c4d8b33ba20896b76b6558f061bc6460ff,
n_tty: Factor LNEXT processing from per-char i/o path,
mistakenly inlined the non-inline alias, n_tty_receive_char(),
for the inline function, n_tty_receive_char_inline().

As n_tty_receive_char() is intended for slow-path char
processing only, un-inline it.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 446ccfd..86de3d0 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1420,7 +1420,7 @@ n_tty_receive_char_inline(struct tty_struct *tty, unsigned char c)
 	put_tty_queue(c, ldata);
 }
 
-static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
+static void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 {
 	n_tty_receive_char_inline(tty, c);
 }
-- 
1.8.1.2

[PATCH v2 tty-next 3/8] n_tty: Un-inline slow-path n_tty_receive_char_closing()

[Peter Hurley]

Although n_tty_receive_char_closing() only has one call-site,
let the compiler inline instead.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 86de3d0..f8a76d3 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1445,8 +1445,7 @@ n_tty_receive_char_fast(struct tty_struct *tty, unsigned char c)
 	put_tty_queue(c, ldata);
 }
 
-static inline void
-n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c)
+static void n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c)
 {
 	if (I_ISTRIP(tty))
 		c &= 0x7f;
-- 
1.8.1.2

[PATCH v2 tty-next 4/8] n_tty: Refactor PARMRK doubling checks

[Peter Hurley]

Perform PARMRK doubling checks explicitly; remove ternary idiom
and local variable.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index f8a76d3..008b6e5 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1260,7 +1260,6 @@ static int
 n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
 {
 	struct n_tty_data *ldata = tty->disc_data;
-	int parmrk;
 
 	if (I_IXON(tty)) {
 		if (c == START_CHAR(tty)) {
@@ -1345,8 +1344,6 @@ n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
 		}
 		if ((c == EOL_CHAR(tty)) ||
 		    (c == EOL2_CHAR(tty) && L_IEXTEN(tty))) {
-			parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty))
-				 ? 1 : 0;
 			/*
 			 * XXX are EOL_CHAR and EOL2_CHAR echoed?!?
 			 */
@@ -1361,7 +1358,7 @@ n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
 			 * XXX does PARMRK doubling happen for
 			 * EOL_CHAR and EOL2_CHAR?
 			 */
-			if (parmrk)
+			if (c == (unsigned char) '\377' && I_PARMRK(tty))
 				put_tty_queue(c, ldata);
 
 handle_newline:
@@ -1375,7 +1372,6 @@ handle_newline:
 		}
 	}
 
-	parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
 	if (L_ECHO(tty)) {
 		finish_erasing(ldata);
 		if (c == '\n')
@@ -1389,7 +1385,8 @@ handle_newline:
 		commit_echoes(tty);
 	}
 
-	if (parmrk)
+	/* PARMRK doubling check */
+	if (c == (unsigned char) '\377' && I_PARMRK(tty))
 		put_tty_queue(c, ldata);
 
 	put_tty_queue(c, ldata);
@@ -1400,7 +1397,6 @@ static inline void
 n_tty_receive_char_inline(struct tty_struct *tty, unsigned char c)
 {
 	struct n_tty_data *ldata = tty->disc_data;
-	int parmrk;
 
 	if (tty->stopped && !tty->flow_stopped && I_IXON(tty) && I_IXANY(tty)) {
 		start_tty(tty);
@@ -1414,8 +1410,8 @@ n_tty_receive_char_inline(struct tty_struct *tty, unsigned char c)
 		echo_char(c, tty);
 		commit_echoes(tty);
 	}
-	parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
-	if (parmrk)
+	/* PARMRK doubling check */
+	if (c == (unsigned char) '\377' && I_PARMRK(tty))
 		put_tty_queue(c, ldata);
 	put_tty_queue(c, ldata);
 }
-- 
1.8.1.2

[PATCH v2 tty-next 5/8] n_tty: Refactor input_available_p() by call site

[Peter Hurley]

Distinguish if caller is n_tty_poll() or n_tty_read(), and
set the read/wakeup threshold accordingly.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 008b6e5..aae171e 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1881,14 +1881,15 @@ static int n_tty_open(struct tty_struct *tty)
 	return 0;
 }
 
-static inline int input_available_p(struct tty_struct *tty, int amt)
+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) : 1;
 
 	if (ldata->icanon && !L_EXTPROC(tty)) {
 		if (ldata->canon_head != ldata->read_tail)
 			return 1;
-	} else if (read_cnt(ldata) >= (amt ? amt : 1))
+	} else if (read_cnt(ldata) >= amt)
 		return 1;
 
 	return 0;
@@ -2393,7 +2394,7 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 
 	poll_wait(file, &tty->read_wait, wait);
 	poll_wait(file, &tty->write_wait, wait);
-	if (input_available_p(tty, TIME_CHAR(tty) ? 0 : MIN_CHAR(tty)))
+	if (input_available_p(tty, 1))
 		mask |= POLLIN | POLLRDNORM;
 	if (tty->packet && tty->link->ctrl_status)
 		mask |= POLLPRI | POLLIN | POLLRDNORM;
-- 
1.8.1.2

[PATCH v2 tty-next 6/8] n_tty: Only perform wakeups for waiters

[Peter Hurley]

Only wakeup the _waiting_ reader, polls and/or writer(s).

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index aae171e..5254082 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -275,7 +275,8 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 			return;
 		n_tty_set_room(tty);
 		n_tty_write_wakeup(tty->link);
-		wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
+		if (waitqueue_active(&tty->link->write_wait))
+			wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
 		return;
 	}
 
@@ -350,7 +351,8 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
 	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (tty->link->packet) {
 		tty->ctrl_status |= TIOCPKT_FLUSHREAD;
-		wake_up_interruptible(&tty->link->read_wait);
+		if (waitqueue_active(&tty->link->read_wait))
+			wake_up_interruptible(&tty->link->read_wait);
 	}
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 }
@@ -1158,7 +1160,8 @@ static void n_tty_receive_break(struct tty_struct *tty)
 		put_tty_queue('\0', ldata);
 	}
 	put_tty_queue('\0', ldata);
-	wake_up_interruptible(&tty->read_wait);
+	if (waitqueue_active(&tty->read_wait))
+		wake_up_interruptible(&tty->read_wait);
 }
 
 /**
@@ -1216,7 +1219,8 @@ static void n_tty_receive_parity_error(struct tty_struct *tty, unsigned char c)
 		put_tty_queue('\0', ldata);
 	else
 		put_tty_queue(c, ldata);
-	wake_up_interruptible(&tty->read_wait);
+	if (waitqueue_active(&tty->read_wait))
+		wake_up_interruptible(&tty->read_wait);
 }
 
 static void
@@ -1811,8 +1815,10 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 		start_tty(tty);
 
 	/* The termios change make the tty ready for I/O */
-	wake_up_interruptible(&tty->write_wait);
-	wake_up_interruptible(&tty->read_wait);
+	if (waitqueue_active(&tty->write_wait))
+		wake_up_interruptible(&tty->write_wait);
+	if (waitqueue_active(&tty->read_wait))
+		wake_up_interruptible(&tty->read_wait);
 }
 
 /**
-- 
1.8.1.2

[PATCH v2 tty-next 7/8] n_tty: Extend debug tracing

[Peter Hurley]

Add configurable trace support to allow tracing with dev_dbg,
pr_dbg, or trace_printk. Instrument .receive_buf() and read()
paths with traces to aid in debugging flow control changes.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 79 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 5254082..26ae9d7 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -80,12 +80,18 @@
 #define ECHO_BLOCK		256
 #define ECHO_DISCARD_WATERMARK	N_TTY_BUF_SIZE - (ECHO_BLOCK + 32)
 
+#define n_tty_trace_facility trace_printk
+#define _n_tty_trace(tty, f, args...)					       \
+	do {								       \
+		char buf[64];						       \
+		n_tty_trace_facility("[%s] " f, tty_name(tty, buf), ##args);   \
+	} while (0)
 
 #undef N_TTY_TRACE
 #ifdef N_TTY_TRACE
-# define n_tty_trace(f, args...)	trace_printk(f, ##args)
+# define n_tty_trace(tty, f, args...)	_n_tty_trace(tty, f, args)
 #else
-# define n_tty_trace(f, args...)
+# define n_tty_trace(tty, f, args...)	do { } while (0)
 #endif
 
 struct n_tty_data {
@@ -179,8 +185,13 @@ static int receive_room(struct tty_struct *tty)
 	 * that erase characters will be handled.  Other excess
 	 * characters will be beeped.
 	 */
-	if (left <= 0)
+	if (left <= 0) {
 		left = ldata->icanon && ldata->canon_head == ldata->read_tail;
+		if (left) {
+			n_tty_trace(tty, "overflow, head:%zu tail:%zu\n",
+				    ldata->read_head, ldata->read_tail);
+		}
+	}
 
 	return left;
 }
@@ -200,11 +211,14 @@ static int receive_room(struct tty_struct *tty)
 static void n_tty_set_room(struct tty_struct *tty)
 {
 	struct n_tty_data *ldata = tty->disc_data;
+	int avail;
 
 	/* Did this open up the receive buffer? We may need to flip */
-	if (unlikely(ldata->no_room) && receive_room(tty)) {
+	if (unlikely(ldata->no_room) && (avail = receive_room(tty))) {
 		ldata->no_room = 0;
 
+		n_tty_trace(tty, "restart worker (avail = %d)\n", avail);
+
 		WARN_RATELIMIT(tty->port->itty == NULL,
 				"scheduling with invalid itty\n");
 		/* see if ldisc has been killed - if so, this means that
@@ -240,6 +254,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
+	n_tty_trace(tty, "\n");
+
 	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
 		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }
@@ -254,13 +270,16 @@ static void n_tty_check_throttle(struct tty_struct *tty)
 	 * canonical mode and don't have a newline yet!
 	 */
 	while (1) {
-		int throttled;
+		int throttled, avail;
 		tty_set_flow_change(tty, TTY_THROTTLE_SAFE);
-		if (receive_room(tty) >= TTY_THRESHOLD_THROTTLE)
+		avail = receive_room(tty);
+		if (avail >= TTY_THRESHOLD_THROTTLE)
 			break;
 		throttled = tty_throttle_safe(tty);
-		if (!throttled)
+		if (!throttled) {
+			n_tty_trace(tty, "throttled (avail = %d)/n", avail);
 			break;
+		}
 	}
 	__tty_set_flow_change(tty, 0);
 }
@@ -289,16 +308,19 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 	 */
 
 	while (1) {
-		int unthrottled;
+		int unthrottled, used;
 		tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
-		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
+		used = chars_in_buffer(tty);
+		if (used > TTY_THRESHOLD_UNTHROTTLE)
 			break;
 		if (!tty->count)
 			break;
 		n_tty_set_room(tty);
 		unthrottled = tty_unthrottle_safe(tty);
-		if (!unthrottled)
+		if (!unthrottled) {
+			n_tty_trace(tty, "unthrottled (used = %d)\n", used);
 			break;
+		}
 	}
 	__tty_set_flow_change(tty, 0);
 }
@@ -374,6 +396,9 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
 static void n_tty_flush_buffer(struct tty_struct *tty)
 {
 	down_write(&tty->termios_rwsem);
+
+	n_tty_trace(tty, "\n");
+
 	reset_buffer_flags(tty->disc_data);
 	n_tty_set_room(tty);
 
@@ -770,6 +795,10 @@ static size_t __process_echoes(struct tty_struct *tty)
 	 * of echo overrun before the next commit), then discard enough
 	 * data at the tail to prevent a subsequent overrun */
 	while (ldata->echo_commit - tail >= ECHO_DISCARD_WATERMARK) {
+
+		n_tty_trace(tty, "discard echoes (%zu - %zu > %d)\n",
+			    ldata->echo_commit, tail, ECHO_DISCARD_WATERMARK);
+
 		if (echo_buf(ldata, tail) == ECHO_OP_START) {
 			if (echo_buf(ldata, tail + 1) == ECHO_OP_ERASE_TAB)
 				tail += 3;
@@ -1369,6 +1398,7 @@ handle_newline:
 			set_bit(ldata->read_head & (N_TTY_BUF_SIZE - 1), ldata->read_flags);
 			put_tty_queue(c, ldata);
 			ldata->canon_head = ldata->read_head;
+			n_tty_trace(tty, "canon_head:%zu\n", ldata->canon_head);
 			kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 			if (waitqueue_active(&tty->read_wait))
 				wake_up_interruptible(&tty->read_wait);
@@ -1670,6 +1700,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 
 	if ((!ldata->icanon && (read_cnt(ldata) >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
+		n_tty_trace(tty, "reader wakeup, count:%zd\n", read_cnt(ldata));
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		if (waitqueue_active(&tty->read_wait))
 			wake_up_interruptible(&tty->read_wait);
@@ -1689,6 +1720,8 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
 		room = receive_room(tty);
 		n = min(count, room);
 		if (!n) {
+			if (!room)
+				n_tty_trace(tty, "buffer full\n");
 			if (flow && !room)
 				ldata->no_room = 1;
 			break;
@@ -1705,6 +1738,8 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
 	n_tty_check_throttle(tty);
 	up_read(&tty->termios_rwsem);
 
+	n_tty_trace(tty, "rcvd:%d\n", rcvd);
+
 	return rcvd;
 }
 
@@ -1814,6 +1849,10 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 	if (!I_IXON(tty) && old && (old->c_iflag & IXON) && !tty->flow_stopped)
 		start_tty(tty);
 
+	n_tty_trace(tty, "mode:%s tail:%zu canon:%zu head:%zu\n",
+		    ldata->icanon ? "canon" : "raw", ldata->read_tail,
+		    ldata->canon_head, ldata->read_head);
+
 	/* The termios change make the tty ready for I/O */
 	if (waitqueue_active(&tty->write_wait))
 		wake_up_interruptible(&tty->write_wait);
@@ -1893,10 +1932,14 @@ static inline int input_available_p(struct tty_struct *tty, int poll)
 	int amt = poll && !TIME_CHAR(tty) ? MIN_CHAR(tty) : 1;
 
 	if (ldata->icanon && !L_EXTPROC(tty)) {
-		if (ldata->canon_head != ldata->read_tail)
+		if (ldata->canon_head != ldata->read_tail) {
+			n_tty_trace(tty, "line avail\n");
 			return 1;
-	} else if (read_cnt(ldata) >= amt)
+		}
+	} else if (read_cnt(ldata) >= amt) {
+		n_tty_trace(tty, "avail:%zu amt:%d\n", read_cnt(ldata), amt);
 		return 1;
+	}
 
 	return 0;
 }
@@ -1993,8 +2036,8 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1);
 	size = min_t(size_t, tail + n, N_TTY_BUF_SIZE);
 
-	n_tty_trace("%s: nr:%zu tail:%zu n:%zu size:%zu\n",
-		    __func__, *nr, tail, n, size);
+	n_tty_trace(tty, "nr:%zu tail:%zu n:%zu size:%zu\n",
+		    *nr, tail, n, size);
 
 	eol = find_next_bit(ldata->read_flags, size, tail);
 	more = n - (size - tail);
@@ -2018,8 +2061,8 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 		eof_push = !n && ldata->read_tail != ldata->line_start;
 	}
 
-	n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
-		    __func__, eol, found, n, c, size, more);
+	n_tty_trace(tty, "eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
+		    eol, found, n, c, size, more);
 
 	if (n > size) {
 		ret = copy_to_user(*b, read_buf_addr(ldata, tail), size);
@@ -2211,6 +2254,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 				n_tty_set_room(tty);
 				up_read(&tty->termios_rwsem);
 
+				n_tty_trace(tty, "blocking\n");
+
 				timeout = schedule_timeout(timeout);
 
 				down_read(&tty->termios_rwsem);
@@ -2268,6 +2313,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 	if (b - buf)
 		retval = b - buf;
 
+	n_tty_trace(tty, "ret:%zd\n", retval);
+
 	return retval;
 }
 
-- 
1.8.1.2

Re: [PATCH v2 tty-next 7/8] n_tty: Extend debug tracing

[Greg Kroah-Hartman]

On Mon, Dec 02, 2013 at 02:24:47PM -0500, Peter Hurley wrote:
> Add configurable trace support to allow tracing with dev_dbg,
> pr_dbg, or trace_printk. Instrument .receive_buf() and read()
> paths with traces to aid in debugging flow control changes.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/n_tty.c | 79 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 63 insertions(+), 16 deletions(-)

If we are going to do tracing in the tty layer, why not use the
in-kernel tracing functionality instead of having this mis-mash of
different ways to get tracing data out?

I'd much rather prefer that, so I've stopped applying things in this
series at this point, I've applied the 6 previous ones.

thanks,

greg k-h

[PATCH v2 tty-next 8/8] n_tty: Trace echo output with separate trace configuration

[Peter Hurley]

Add trace support for echo output but leave it separately selectable/
buildable from the base trace support.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 26ae9d7..bd01d97 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -94,6 +94,13 @@
 # define n_tty_trace(tty, f, args...)	do { } while (0)
 #endif
 
+#undef N_TTY_TRACE_ECHOES
+#ifdef N_TTY_TRACE_ECHOES
+# define n_tty_trace_echoes(tty, f, args...)	_n_tty_trace(tty, f, args)
+#else
+# define n_tty_trace_echoes(tty, f, args...)	do { } while (0)
+#endif
+
 struct n_tty_data {
 	/* producer-published */
 	size_t read_head;
@@ -791,13 +798,18 @@ static size_t __process_echoes(struct tty_struct *tty)
 		}
 	}
 
+	n_tty_trace_echoes(tty, "echoes:%zu,%zu commit:%zu head:%zu\n",
+			   ldata->echo_tail, tail, ldata->echo_commit,
+			   ldata->echo_head);
+
 	/* If the echo buffer is nearly full (so that the possibility exists
 	 * of echo overrun before the next commit), then discard enough
 	 * data at the tail to prevent a subsequent overrun */
 	while (ldata->echo_commit - tail >= ECHO_DISCARD_WATERMARK) {
 
-		n_tty_trace(tty, "discard echoes (%zu - %zu > %d)\n",
-			    ldata->echo_commit, tail, ECHO_DISCARD_WATERMARK);
+		n_tty_trace_echoes(tty, "discard echoes (%zu - %zu > %d)\n",
+				   ldata->echo_commit, tail,
+				   ECHO_DISCARD_WATERMARK);
 
 		if (echo_buf(ldata, tail) == ECHO_OP_START) {
 			if (echo_buf(ldata, tail + 1) == ECHO_OP_ERASE_TAB)
-- 
1.8.1.2