More n_tty fixes

PATCH Superseded Peter Hurley 2 participants 20 messages Dec 12, 2015 – Jan 10, 2016 TTY layer and serial drivers

lore

Share & tools

lore

Peter Hurley

author

2015-12-12 22:16 UTC #0 lore raw

cover More n_tty fixes

Hi Greg,

This series collects up several fixes for N_TTY.

The first patch fixes read() when VMIN > 0 & VTIME = 0 (so called "record mode"). By ripping out a marginal optimization, the overall code is greatly simplified and "record mode" read()s won't hang :)

Patch 2 removes the pointless fasync() notification to line disciplines. Patches 3,4 & 5 fix hangup races with enabling/disabling signal-driven i/o. Patch 6 is a minor cleanup for a condition test that can never be true.

Regards,

Peter Hurley (6):
n_tty: Always wake up read()/poll() if new input tty, n_tty: Remove fasync() ldisc notification
tty: Add fasync() hung up file operation
tty: Fix ioctl(FIOASYNC) on hungup file
n_tty: Fix stuck write wakeup
n_tty: Remove tty count checks from unthrottle

 drivers/tty/n_tty.c       | 46 ++++------------------------------------------
 drivers/tty/tty_io.c      | 19 +++++++++----------
 include/linux/tty_ldisc.h |  6 ------
 3 files changed, 13 insertions(+), 58 deletions(-)

Signature · 1 line

2.6.3

Peter Hurley

author

2015-12-12 22:16 UTC #1 lore raw

PATCH 1/6 n_tty: Always wake up read()/poll() if new input

A read() in non-canonical mode when VMIN > 0 and VTIME == 0 does not complete until at least VMIN chars have been read (or the user buffer is full). In this infrequent read mode, n_tty_read() attempts to reduce wakeups by computing the amount of data still necessary to complete the read (minimum_to_wake) and only waking the read()/poll() when that much unread data has been processed. This is the only read mode for which new data does not necessarily generate a wakeup.

However, this optimization is broken and commonly leads to hung reads even though the necessary amount of data has been received. Since the optimization is of marginal value anyway, just remove the whole thing. This also remedies a race between a concurrent poll() and read() in this mode, where the poll() can reset the minimum_to_wake of the read() (and vice versa).

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 33 ++-------------------------------
 1 file changed, 2 insertions(+), 31 deletions(-)

drivers/tty/n_tty.c +2 ~~-31~~

index 4e1867a..16ba07b 100644
@@ -113,7 +113,6 @@ struct n_tty_data {
 	DECLARE_BITMAP(read_flags, N_TTY_BUF_SIZE);
 	unsigned char echo_buf[N_TTY_BUF_SIZE];
 
-	int minimum_to_wake;
 	int closing;
 
 	/* consumer-published */

@@ -1632,7 +1631,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	/* publish read_head to consumer */
 	smp_store_release(&ldata->commit_head, ldata->read_head);
 
-	if ((read_cnt(ldata) >= ldata->minimum_to_wake) || L_EXTPROC(tty)) {
+	if (read_cnt(ldata)) {
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		wake_up_interruptible_poll(&tty->read_wait, POLLIN);
 	}

@@ -1899,7 +1898,6 @@ static int n_tty_open(struct tty_struct *tty)
 	reset_buffer_flags(tty->disc_data);
 	ldata->column = 0;
 	ldata->canon_column = 0;
-	ldata->minimum_to_wake = 1;
 	ldata->num_overrun = 0;
 	ldata->no_room = 0;
 	ldata->lnext = 0;

@@ -2162,14 +2160,9 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
 			time = (HZ / 10) * TIME_CHAR(tty);
-			if (time)
-				ldata->minimum_to_wake = 1;
-			else if (!waitqueue_active(&tty->read_wait) ||
-				 (ldata->minimum_to_wake > minimum))
-				ldata->minimum_to_wake = minimum;
 		} else {
 			timeout = (HZ / 10) * TIME_CHAR(tty);
-			ldata->minimum_to_wake = minimum = 1;
+			minimum = 1;
 		}
 	}

@@ -2196,10 +2189,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 			break;
 		}
 
-		if (((minimum - (b - buf)) < ldata->minimum_to_wake) &&
-		    ((minimum - (b - buf)) >= 1))
-			ldata->minimum_to_wake = (minimum - (b - buf));
-
 		done = check_other_done(tty);
 
 		if (!input_available_p(tty, 0)) {

@@ -2265,9 +2254,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 	up_read(&tty->termios_rwsem);
 
 	remove_wait_queue(&tty->read_wait, &wait);
-	if (!waitqueue_active(&tty->read_wait))
-		ldata->minimum_to_wake = minimum;
-
 	mutex_unlock(&ldata->atomic_read_lock);
 
 	if (b - buf)

@@ -2402,7 +2388,6 @@ break_out:
 static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 							poll_table *wait)
 {
-	struct n_tty_data *ldata = tty->disc_data;
 	unsigned int mask = 0;
 
 	poll_wait(file, &tty->read_wait, wait);

@@ -2415,12 +2400,6 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 		mask |= POLLPRI | POLLIN | POLLRDNORM;
 	if (tty_hung_up_p(file))
 		mask |= POLLHUP;
-	if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
-		if (MIN_CHAR(tty) && !TIME_CHAR(tty))
-			ldata->minimum_to_wake = MIN_CHAR(tty);
-		else
-			ldata->minimum_to_wake = 1;
-	}
 	if (tty->ops->write && !tty_is_writelocked(tty) &&
 			tty_chars_in_buffer(tty) < WAKEUP_CHARS &&
 			tty_write_room(tty) > 0)

@@ -2471,14 +2450,6 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 
 static void n_tty_fasync(struct tty_struct *tty, int on)
 {
-	struct n_tty_data *ldata = tty->disc_data;
-
-	if (!waitqueue_active(&tty->read_wait)) {
-		if (on)
-			ldata->minimum_to_wake = 1;
-		else if (!tty->fasync)
-			ldata->minimum_to_wake = N_TTY_BUF_SIZE;
-	}
 }
 
 static void n_tty_closing(struct tty_struct *tty)

Signature · 1 line

2.6.3

Johannes Stezenbach

2015-12-13 14:49 UTC #2 lore raw

Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input

Hi Peter,

Quoted 14 lines · Peter Hurley

A read() in non-canonical mode when VMIN > 0 and VTIME == 0 does not complete until at least VMIN chars have been read (or the user buffer is full). In this infrequent read mode, n_tty_read() attempts to reduce wakeups by computing the amount of data still necessary to complete the read (minimum_to_wake) and only waking the read()/poll() when that much unread data has been processed. This is the only read mode for which new data does not necessarily generate a wakeup.
However, this optimization is broken and commonly leads to hung reads even though the necessary amount of data has been received. Since the optimization is of marginal value anyway, just remove the whole thing. This also remedies a race between a concurrent poll() and read() in this mode, where the poll() can reset the minimum_to_wake of the read() (and vice versa).

...

drivers/tty/n_tty.c static void __receive_buf(struct tty_struct *tty, const unsigned char *cp, · line 1637

Quoted 9 lines

drivers/tty/n_tty.c +1 -1

@@ -1632,7 +1631,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	/* publish read_head to consumer */
 	smp_store_release(&ldata->commit_head, ldata->read_head);
 
-	if ((read_cnt(ldata) >= ldata->minimum_to_wake) || L_EXTPROC(tty)) {
+	if (read_cnt(ldata)) {
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		wake_up_interruptible_poll(&tty->read_wait, POLLIN);
 	}

Your patch looks fine, I just want to mention that there was some undocumented behaviour for async IO to take VMIN into account for deciding when to send SIGIO, but it was implemented incorrectly because minimum_to_wake was only updated in read() and poll(), not directly by the tcsetattr() ioctl. I think your change does the right thing to fix this case, too. I had to debug some proprietary code which dynamically changed VMIN based on expected message size and thus sometimes wasn't woken up, in the end we decided to keep VMIN=1 to solve it.

Thanks,
Johannes

Peter Hurley

author

2015-12-13 19:53 UTC #3 lore raw

Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input

Hi Johannes,

Quoted 22 lines · Johannes Stezenbach

Hi Peter,
Quoted 14 lines · Peter Hurley
A read() in non-canonical mode when VMIN > 0 and VTIME == 0 does not complete until at least VMIN chars have been read (or the user buffer is full). In this infrequent read mode, n_tty_read() attempts to reduce wakeups by computing the amount of data still necessary to complete the read (minimum_to_wake) and only waking the read()/poll() when that much unread data has been processed. This is the only read mode for which new data does not necessarily generate a wakeup.
However, this optimization is broken and commonly leads to hung reads even though the necessary amount of data has been received. Since the optimization is of marginal value anyway, just remove the whole thing. This also remedies a race between a concurrent poll() and read() in this mode, where the poll() can reset the minimum_to_wake of the read() (and vice versa).
...
Quoted 4 lines
drivers/tty/n_tty.c +0 -0
@@ -1632,7 +1631,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	/* publish read_head to consumer */
 	smp_store_release(&ldata->commit_head, ldata->read_head);
 

Quoted 16 lines

drivers/tty/n_tty.c static void __receive_buf(struct tty_struct *tty, const unsigned char *cp, · line 1637
Quoted 5 lines
drivers/tty/n_tty.c +1 -1
-	if ((read_cnt(ldata) >= ldata->minimum_to_wake) || L_EXTPROC(tty)) {
+	if (read_cnt(ldata)) {
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		wake_up_interruptible_poll(&tty->read_wait, POLLIN);
 	}
Your patch looks fine, I just want to mention that there was some undocumented behaviour for async IO to take VMIN into account for deciding when to send SIGIO, but it was implemented incorrectly because minimum_to_wake was only updated in read() and poll(), not directly by the tcsetattr() ioctl. I think your change does the right thing to fix this case, too. I had to debug some proprietary code which dynamically changed VMIN based on expected message size and thus sometimes wasn't woken up, in the end we decided to keep VMIN=1 to solve it.

I considered re-implementing the minimum_to_wake mechanism (in a race-free way) but I'm not sure it's worth the effort (either in initial implementation time or in maintenance head-ache). Now that termios changes are serialized with an active reader and the input worker, it is at least possible.

Regards,
Peter Hurley

Peter Hurley

author

2015-12-12 22:16 UTC #4 lore raw

PATCH 2/6 tty, n_tty: Remove fasync() ldisc notification

In reply to Peter Hurley

Only the N_TTY line discipline implements the signal-driven i/o notification enabled/disabled by fcntl(F_SETFL, O_ASYNC). The ldisc fasync() notification is sent to the ldisc when the enable state has changed (the tty core is notified via the fasync() VFS file operation).

The N_TTY line discipline used the enable state to change the wakeup condition (minimum_to_wake = 1) for notifying the signal handler i/o is available. However, just the presence of data is sufficient and necessary to signal i/o is available, so changing minimum_to_wake is unnecessary (and creates a race condition with read() and poll() which may be concurrently updating minimum_to_wake).

Furthermore, since the kill_fasync() VFS helper performs no action if the fasync list is empty, calling unconditionally is preferred; if signal driven i/o just has been disabled, no signal will be sent by kill_fasync() anyway so notification of the change via the ldisc fasync() method is superfluous.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c       | 5 -----
 drivers/tty/tty_io.c      | 8 --------
 include/linux/tty_ldisc.h | 6 ------
 3 files changed, 19 deletions(-)

drivers/tty/n_tty.c +0 -5

index 16ba07b..e695f8f 100644
@@ -2448,10 +2448,6 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 	}
 }
 
-static void n_tty_fasync(struct tty_struct *tty, int on)
-{
-}
-
 static void n_tty_closing(struct tty_struct *tty)
 {
 	struct n_tty_data *ldata = tty->disc_data;

@@ -2472,7 +2468,6 @@ static struct tty_ldisc_ops n_tty_ops = {
 	.poll            = n_tty_poll,
 	.receive_buf     = n_tty_receive_buf,
 	.write_wakeup    = n_tty_write_wakeup,
-	.fasync		 = n_tty_fasync,
 	.receive_buf2	 = n_tty_receive_buf2,
 	.closing	 = n_tty_closing,
 };

drivers/tty/tty_io.c +0 -8

index 5c8f519..df7a3a9 100644
@@ -2209,7 +2209,6 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
 static int __tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
-	struct tty_ldisc *ldisc;
 	unsigned long flags;
 	int retval = 0;

@@ -2220,13 +2219,6 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 	if (retval <= 0)
 		goto out;
 
-	ldisc = tty_ldisc_ref(tty);
-	if (ldisc) {
-		if (ldisc->ops->fasync)
-			ldisc->ops->fasync(tty, on);
-		tty_ldisc_deref(ldisc);
-	}
-
 	if (on) {
 		enum pid_type type;
 		struct pid *pid;

include/linux/tty_ldisc.h +0 -6

index db0abe56..08fd06a 100644
@@ -98,11 +98,6 @@
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
  *
- * void (*fasync)(struct tty_struct *, int on)
- *
- *	Notify line discipline when signal-driven I/O is enabled or
- *	disabled.
- *
  * void (*dcd_change)(struct tty_struct *tty, unsigned int status)
  *
  *	Tells the discipline that the DCD pin has changed its status.

@@ -210,7 +205,6 @@ struct tty_ldisc_ops {
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
-	void	(*fasync)(struct tty_struct *tty, int on);
 	int	(*receive_buf2)(struct tty_struct *, const unsigned char *cp,
 				char *fp, int count);
 	void	(*closing)(struct tty_struct *tty);

Signature · 1 line

2.6.3

Peter Hurley

author

2015-12-12 22:16 UTC #5 lore raw

PATCH 3/6 tty: Add fasync() hung up file operation

In reply to Peter Hurley

VFS uses a two-stage check-and-call method for invoking file_operations methods, without explicitly snapshotting either the file_operations ptr or the function ptr. Since the tty core is one of the few VFS users that changes the f_op file_operations ptr of the file descriptor (when the tty has been hung up), and since the likelihood of the compiler generating a reload of either f_op or the function ptr is basically nil, just define a hung up fasync() file operation that returns an error.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/tty/tty_io.c +6 -0

index df7a3a9..e9b7591 100644
@@ -468,6 +468,11 @@ static long hung_up_tty_compat_ioctl(struct file *file,
 	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
 }
 
+static int hung_up_tty_fasync(int fd, struct file *file, int on)
+{
+	return -ENOTTY;
+}
+
 static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
 	.read		= tty_read,

@@ -500,6 +505,7 @@ static const struct file_operations hung_up_tty_fops = {
 	.unlocked_ioctl	= hung_up_tty_ioctl,
 	.compat_ioctl	= hung_up_tty_compat_ioctl,
 	.release	= tty_release,
+	.fasync		= hung_up_tty_fasync,
 };
 
 static DEFINE_SPINLOCK(redirect_lock);

Signature · 1 line

2.6.3

Peter Hurley

author

2015-12-12 22:18 UTC #6 lore raw

PATCH 4/6 tty: Fix ioctl(FIOASYNC) on hungup file

In reply to Peter Hurley

A small race window exists which allows signal-driven async i/o to be enabled for the tty when the file ptr has already been hungup and signal-driven i/o has been disabled:

CPU 0 CPU 1
----- ------
ioctl_fioasync(on)
filp->f_op->fasync(on) __tty_hangup()

    tty_fasync(on)                     tty_lock()
      tty_lock()                       ...
        .                              filp->f_op = &hung_up_tty_fops;
      (waiting)                       __tty_fasync(off)
        .                              tty_unlock()
      /* gets tty lock  */
      /* enables FASYNC */

Check the tty has not been hungup while holding tty_lock.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/tty/tty_io.c +3 -2

index e9b7591..19cec0e 100644
@@ -2250,10 +2250,11 @@ out:
 static int tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
-	int retval;
+	int retval = -ENOTTY;
 
 	tty_lock(tty);
-	retval = __tty_fasync(fd, filp, on);
+	if (!tty_hung_up_p(filp))
+		retval = __tty_fasync(fd, filp, on);
 	tty_unlock(tty);
 
 	return retval;

Signature · 1 line

2.6.3

Peter Hurley

author

2015-12-12 22:17 UTC #7 lore raw

PATCH 5/6 n_tty: Fix stuck write wakeup

In reply to Peter Hurley

If signal-driven i/o is disabled while write wakeup is pending (ie., n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and will cause tty_wakeup() to always call n_tty_write_wakeup.

Unconditionally clear the write wakeup, and since kill_fasync() already checks if the fasync ptr is null, call kill_fasync() unconditionally as well.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tty/n_tty.c +2 -2

index e695f8f..5034da0 100644
@@ -230,8 +230,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
-	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
-		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
+	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }
 
 static void n_tty_check_throttle(struct tty_struct *tty)

Signature · 1 line

2.6.3

Johannes Stezenbach

2015-12-13 15:18 UTC #8 lore raw

Re: [PATCH 5/6] n_tty: Fix stuck write wakeup

Hi Peter,

Quoted 8 lines · Peter Hurley

If signal-driven i/o is disabled while write wakeup is pending (ie., n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and will cause tty_wakeup() to always call n_tty_write_wakeup.
Unconditionally clear the write wakeup, and since kill_fasync() already checks if the fasync ptr is null, call kill_fasync() unconditionally as well.

...

Review static ssize_t chars_in_buffer(struct tty_struct *tty) · line 235

Quoted 9 lines

(quoted diff) +2 -2

@@ -230,8 +230,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
-	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
-		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
+	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }

There is a related bug that I meant to send a patch, but I never got around because the issue was found with proprietary userspace and ancient kernel. Maybe you could take care of it? The patch might not apply cleanly after your recent changes or might even be invalid now, please check.

Thanks,
Johannes

---
tty: n_tty: fix SIGIO for output

According to fcntl(2), "a SIGIO signal is sent whenever input or output becomes possible on that file descriptor", i.e. after the output buffer was full and now has space for new data. But in fact SIGIO is sent after every write.

n_tty_write() should set TTY_DO_WRITE_WAKEUP only when not all data could be written to the buffer.

Signed-off-by Johannes Stezenbach <[email protected]>

char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 renamed from char/n_tty.c.orig 2015-11-02 22:26:04.124227148 +0100 +2 -1

@@ -1925,6 +1925,7 @@ static ssize_t n_tty_write(struct tty_st
        DECLARE_WAITQUEUE(wait, current);
        int c;
        ssize_t retval = 0;
+       size_t count = nr;
 
        /* Job control check -- must be done at start (POSIX.1 7.1.1.4). */
        if (L_TOSTOP(tty) && file->f_op->write != redirected_tty_write) {

@@ -1991,7 +1992,7 @@ static ssize_t n_tty_write(struct tty_st
 break_out:
        __set_current_state(TASK_RUNNING);
        remove_wait_queue(&tty->write_wait, &wait);
-       if (b - buf != nr && tty->fasync)
+       if (b - buf != count && tty->fasync)
                set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
        return (b - buf) ? b - buf : retval;
 }

Peter Hurley

author

2015-12-13 18:38 UTC #9 lore raw

Re: [PATCH 5/6] n_tty: Fix stuck write wakeup

Quoted 14 lines · Johannes Stezenbach

Hi Peter,
Quoted 8 lines · Peter Hurley
If signal-driven i/o is disabled while write wakeup is pending (ie., n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and will cause tty_wakeup() to always call n_tty_write_wakeup.
Unconditionally clear the write wakeup, and since kill_fasync() already checks if the fasync ptr is null, call kill_fasync() unconditionally as well.
...
Quoted 2 lines
(quoted diff) +0 -0
@@ -230,8 +230,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 

Quoted 13 lines

Review static ssize_t chars_in_buffer(struct tty_struct *tty) · line 235
Quoted 7 lines
(quoted diff) +2 -2
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
-	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
-		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
+	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }
There is a related bug that I meant to send a patch, but I never got around because the issue was found with proprietary userspace and ancient kernel. Maybe you could take care of it? The patch might not apply cleanly after your recent changes or might even be invalid now, please check.

Thanks for the patch, Johannes!

Yes, the patch below is still required to prevent excessive SIGIO (and to prevent missed SIGIO when the amount actually copied just happens to be exactly the amount left to be copied).

I made some comments in the patch; can you re-submit with those changes and the patch title in the subject? Or I'd happy to re-work it and send it to Greg if you'd prefer; just let me know.

Regards,
Peter Hurley

Quoted 16 lines

---
tty: n_tty: fix SIGIO for output
According to fcntl(2), "a SIGIO signal is sent whenever input or output becomes possible on that file descriptor", i.e. after the output buffer was full and now has space for new data. But in fact SIGIO is sent after every write.
n_tty_write() should set TTY_DO_WRITE_WAKEUP only when not all data could be written to the buffer.
Signed-off-by Johannes Stezenbach <[email protected]>

char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 static ssize_t n_tty_write(struct tty_st · line 1928

Quoted 4 lines

char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 renamed from char/n_tty.c.orig 2015-11-02 22:26:04.124227148 +0100 +1 -0
@@ -1925,6 +1925,7 @@ static ssize_t n_tty_write(struct tty_st
        DECLARE_WAITQUEUE(wait, current);
        int c;
        ssize_t retval = 0;
+       size_t count = nr;

'count' isn't required because after exiting the write loop, 'nr' will be the remainder still to write so ...

char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 static ssize_t n_tty_write(struct tty_st · line 1995

Quoted 9 lines

char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 renamed from char/n_tty.c.orig 2015-11-02 22:26:04.124227148 +0100 +1 -1

        /* Job control check -- must be done at start (POSIX.1 7.1.1.4). */
        if (L_TOSTOP(tty) && file->f_op->write != redirected_tty_write) {

@@ -1991,7 +1992,7 @@ static ssize_t n_tty_write(struct tty_st
 break_out:
        __set_current_state(TASK_RUNNING);
        remove_wait_queue(&tty->write_wait, &wait);
-       if (b - buf != nr && tty->fasync)
+       if (b - buf != count && tty->fasync)

... this can be

	if (nr && tty->fasync)
		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);

Quoted 5 lines

char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 renamed from char/n_tty.c.orig 2015-11-02 22:26:04.124227148 +0100 +0 -0
                set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
        return (b - buf) ? b - buf : retval;
 }

Johannes Stezenbach

2015-12-13 19:27 UTC #10 lore raw

Re: [PATCH 5/6] n_tty: Fix stuck write wakeup

Quoted 10 lines · Peter Hurley

Quoted 6 lines · Johannes Stezenbach
There is a related bug that I meant to send a patch, but I never got around because the issue was found with proprietary userspace and ancient kernel. Maybe you could take care of it? The patch might not apply cleanly after your recent changes or might even be invalid now, please check.
Thanks for the patch, Johannes!

Quoted 7 lines

Yes, the patch below is still required to prevent excessive SIGIO (and to prevent missed SIGIO when the amount actually copied just happens to be exactly the amount left to be copied).
I made some comments in the patch; can you re-submit with those changes and the patch title in the subject? Or I'd happy to re-work it and send it to Greg if you'd prefer; just let me know.

Please rework it, currently I'm in lazy bum mode ;-)

Quoted 11 lines

char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 static ssize_t n_tty_write(struct tty_st · line 1995
Quoted 6 lines
char/n_tty.c 2015-11-02 22:26:10.644212115 +0100 renamed from char/n_tty.c.orig 2015-11-02 22:26:04.124227148 +0100 +1 -1
@@ -1991,7 +1992,7 @@ static ssize_t n_tty_write(struct tty_st
 break_out:
        __set_current_state(TASK_RUNNING);
        remove_wait_queue(&tty->write_wait, &wait);
-       if (b - buf != nr && tty->fasync)
+       if (b - buf != count && tty->fasync)
... this can be
	if (nr && tty->fasync)
		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);

Yeah, that's way better.

Thanks,
Johannes

Peter Hurley

author

2015-12-12 22:17 UTC #11 lore raw

PATCH 6/6 n_tty: Remove tty count checks from unthrottle

In reply to Peter Hurley

Since n_tty_check_unthrottle() is only called from n_tty_read() which only originates from a userspace read(), the tty count cannot be 0; the read() guarantees the file descriptor has not yet been released.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/tty/n_tty.c +0 -4

index 5034da0..dd34c5d 100644
@@ -263,8 +263,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 	if (tty->driver->type == TTY_DRIVER_TYPE_PTY) {
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			return;
-		if (!tty->count)
-			return;
 		n_tty_kick_worker(tty);
 		tty_wakeup(tty->link);
 		return;

@@ -283,8 +281,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 		tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			break;
-		if (!tty->count)
-			break;
 		n_tty_kick_worker(tty);
 		unthrottled = tty_unthrottle_safe(tty);
 		if (!unthrottled)

Signature · 1 line

2.6.3

Peter Hurley

author

2016-01-10 05:45 UTC #12 lore raw

v2 cover More n_tty fixes

In reply to Peter Hurley

Changes from v2:
added patch 7 from Johannes Stezenbach which fixes the TTY_DO_WRITE_WAKEUP logic to prevent missed SIGIO and reduce excessive SIGIO

original message follows:

Hi Greg,

This series collects up several fixes for N_TTY.

Peter Hurley (7):
n_tty: Always wake up read()/poll() if new input tty, n_tty: Remove fasync() ldisc notification
tty: Add fasync() hung up file operation
tty: Fix ioctl(FIOASYNC) on hungup file
n_tty: Fix stuck write wakeup
n_tty: Remove tty count checks from unthrottle
tty: n_tty: fix SIGIO for output

 drivers/tty/n_tty.c       | 49 +++++------------------------------------------
 drivers/tty/tty_io.c      | 19 +++++++++---------
 include/linux/tty_ldisc.h |  6 ------
 3 files changed, 14 insertions(+), 60 deletions(-)

Signature · 1 line

2.7.0

Peter Hurley

author

2016-01-10 05:46 UTC #13 lore raw

PATCH v2 1/7 n_tty: Always wake up read()/poll() if new input

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

drivers/tty/n_tty.c +2 ~~-32~~

index 8272069..d04f398 100644
@@ -113,8 +113,6 @@ struct n_tty_data {
 	DECLARE_BITMAP(read_flags, N_TTY_BUF_SIZE);
 	unsigned char echo_buf[N_TTY_BUF_SIZE];
 
-	int minimum_to_wake;
-
 	/* consumer-published */
 	size_t read_tail;
 	size_t line_start;

@@ -1633,7 +1631,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	/* publish read_head to consumer */
 	smp_store_release(&ldata->commit_head, ldata->read_head);
 
-	if ((read_cnt(ldata) >= ldata->minimum_to_wake) || L_EXTPROC(tty)) {
+	if (read_cnt(ldata)) {
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		wake_up_interruptible_poll(&tty->read_wait, POLLIN);
 	}

@@ -1900,7 +1898,6 @@ static int n_tty_open(struct tty_struct *tty)
 	reset_buffer_flags(tty->disc_data);
 	ldata->column = 0;
 	ldata->canon_column = 0;
-	ldata->minimum_to_wake = 1;
 	ldata->num_overrun = 0;
 	ldata->no_room = 0;
 	ldata->lnext = 0;

@@ -2163,14 +2160,9 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
 			time = (HZ / 10) * TIME_CHAR(tty);
-			if (time)
-				ldata->minimum_to_wake = 1;
-			else if (!waitqueue_active(&tty->read_wait) ||
-				 (ldata->minimum_to_wake > minimum))
-				ldata->minimum_to_wake = minimum;
 		} else {
 			timeout = (HZ / 10) * TIME_CHAR(tty);
-			ldata->minimum_to_wake = minimum = 1;
+			minimum = 1;
 		}
 	}

@@ -2197,10 +2189,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 			break;
 		}
 
-		if (((minimum - (b - buf)) < ldata->minimum_to_wake) &&
-		    ((minimum - (b - buf)) >= 1))
-			ldata->minimum_to_wake = (minimum - (b - buf));
-
 		done = check_other_done(tty);
 
 		if (!input_available_p(tty, 0)) {

@@ -2266,9 +2254,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 	up_read(&tty->termios_rwsem);
 
 	remove_wait_queue(&tty->read_wait, &wait);
-	if (!waitqueue_active(&tty->read_wait))
-		ldata->minimum_to_wake = minimum;
-
 	mutex_unlock(&ldata->atomic_read_lock);
 
 	if (b - buf)

@@ -2403,7 +2388,6 @@ break_out:
 static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 							poll_table *wait)
 {
-	struct n_tty_data *ldata = tty->disc_data;
 	unsigned int mask = 0;
 
 	poll_wait(file, &tty->read_wait, wait);

@@ -2416,12 +2400,6 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 		mask |= POLLPRI | POLLIN | POLLRDNORM;
 	if (tty_hung_up_p(file))
 		mask |= POLLHUP;
-	if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
-		if (MIN_CHAR(tty) && !TIME_CHAR(tty))
-			ldata->minimum_to_wake = MIN_CHAR(tty);
-		else
-			ldata->minimum_to_wake = 1;
-	}
 	if (tty->ops->write && !tty_is_writelocked(tty) &&
 			tty_chars_in_buffer(tty) < WAKEUP_CHARS &&
 			tty_write_room(tty) > 0)

@@ -2472,14 +2450,6 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 
 static void n_tty_fasync(struct tty_struct *tty, int on)
 {
-	struct n_tty_data *ldata = tty->disc_data;
-
-	if (!waitqueue_active(&tty->read_wait)) {
-		if (on)
-			ldata->minimum_to_wake = 1;
-		else if (!tty->fasync)
-			ldata->minimum_to_wake = N_TTY_BUF_SIZE;
-	}
 }
 
 static struct tty_ldisc_ops n_tty_ops = {

Signature · 1 line

2.7.0

Peter Hurley

author

2016-01-10 05:46 UTC #14 lore raw

PATCH v2 2/7 tty, n_tty: Remove fasync() ldisc notification

In reply to Peter Hurley

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c       | 5 -----
 drivers/tty/tty_io.c      | 8 --------
 include/linux/tty_ldisc.h | 6 ------
 3 files changed, 19 deletions(-)

drivers/tty/n_tty.c +0 -5

index d04f398..a93e4c7 100644
@@ -2448,10 +2448,6 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 	}
 }
 
-static void n_tty_fasync(struct tty_struct *tty, int on)
-{
-}
-
 static struct tty_ldisc_ops n_tty_ops = {
 	.magic           = TTY_LDISC_MAGIC,
 	.name            = "n_tty",

@@ -2465,7 +2461,6 @@ static struct tty_ldisc_ops n_tty_ops = {
 	.poll            = n_tty_poll,
 	.receive_buf     = n_tty_receive_buf,
 	.write_wakeup    = n_tty_write_wakeup,
-	.fasync		 = n_tty_fasync,
 	.receive_buf2	 = n_tty_receive_buf2,
 };

drivers/tty/tty_io.c +0 -8

index 48281a5..e8b431e 100644
@@ -2219,7 +2219,6 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
 static int __tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
-	struct tty_ldisc *ldisc;
 	unsigned long flags;
 	int retval = 0;

@@ -2230,13 +2229,6 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 	if (retval <= 0)
 		goto out;
 
-	ldisc = tty_ldisc_ref(tty);
-	if (ldisc) {
-		if (ldisc->ops->fasync)
-			ldisc->ops->fasync(tty, on);
-		tty_ldisc_deref(ldisc);
-	}
-
 	if (on) {
 		enum pid_type type;
 		struct pid *pid;

include/linux/tty_ldisc.h +0 -6

index 6101ab8..3971cf0 100644
@@ -98,11 +98,6 @@
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
  *
- * void (*fasync)(struct tty_struct *, int on)
- *
- *	Notify line discipline when signal-driven I/O is enabled or
- *	disabled.
- *
  * void (*dcd_change)(struct tty_struct *tty, unsigned int status)
  *
  *	Tells the discipline that the DCD pin has changed its status.

@@ -202,7 +197,6 @@ struct tty_ldisc_ops {
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
-	void	(*fasync)(struct tty_struct *tty, int on);
 	int	(*receive_buf2)(struct tty_struct *, const unsigned char *cp,
 				char *fp, int count);

Signature · 1 line

2.7.0

Peter Hurley

author

2016-01-10 05:46 UTC #15 lore raw

PATCH v2 3/7 tty: Add fasync() hung up file operation

In reply to Peter Hurley

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/tty/tty_io.c +6 -0

index e8b431e..05b0a9c 100644
@@ -468,6 +468,11 @@ static long hung_up_tty_compat_ioctl(struct file *file,
 	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
 }
 
+static int hung_up_tty_fasync(int fd, struct file *file, int on)
+{
+	return -ENOTTY;
+}
+
 static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
 	.read		= tty_read,

@@ -500,6 +505,7 @@ static const struct file_operations hung_up_tty_fops = {
 	.unlocked_ioctl	= hung_up_tty_ioctl,
 	.compat_ioctl	= hung_up_tty_compat_ioctl,
 	.release	= tty_release,
+	.fasync		= hung_up_tty_fasync,
 };
 
 static DEFINE_SPINLOCK(redirect_lock);

Signature · 1 line

2.7.0

Peter Hurley

author

2016-01-10 05:46 UTC #16 lore raw

PATCH v2 4/7 tty: Fix ioctl(FIOASYNC) on hungup file

In reply to Peter Hurley

A small race window exists which allows signal-driven async i/o to be enabled for the tty when the file ptr has already been hungup and signal-driven i/o has been disabled:

CPU 0 CPU 1
----- ------
ioctl_fioasync(on)
filp->f_op->fasync(on) __tty_hangup()

    tty_fasync(on)                     tty_lock()
      tty_lock()                       ...
        .                              filp->f_op = &hung_up_tty_fops;
      (waiting)                       __tty_fasync(off)
        .                              tty_unlock()
      /* gets tty lock  */
      /* enables FASYNC */

Check the tty has not been hungup while holding tty_lock.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/tty/tty_io.c +3 -2

index 05b0a9c..7113bfa 100644
@@ -2260,10 +2260,11 @@ out:
 static int tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
-	int retval;
+	int retval = -ENOTTY;
 
 	tty_lock(tty);
-	retval = __tty_fasync(fd, filp, on);
+	if (!tty_hung_up_p(filp))
+		retval = __tty_fasync(fd, filp, on);
 	tty_unlock(tty);
 
 	return retval;

Signature · 1 line

2.7.0

Peter Hurley

author

2016-01-10 05:46 UTC #17 lore raw

PATCH v2 5/7 n_tty: Fix stuck write wakeup

In reply to Peter Hurley

Unconditionally clear the write wakeup, and since kill_fasync() already checks if the fasync ptr is null, call kill_fasync() unconditionally as well.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tty/n_tty.c +2 -2

index a93e4c7..086fd99 100644
@@ -228,8 +228,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
-	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
-		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
+	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }
 
 static void n_tty_check_throttle(struct tty_struct *tty)

Signature · 1 line

2.7.0

Peter Hurley

author

2016-01-10 05:46 UTC #18 lore raw

PATCH v2 6/7 n_tty: Remove tty count checks from unthrottle

In reply to Peter Hurley

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/tty/n_tty.c +0 -4

index 086fd99..5ae661c 100644
@@ -261,8 +261,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 	if (tty->driver->type == TTY_DRIVER_TYPE_PTY) {
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			return;
-		if (!tty->count)
-			return;
 		n_tty_kick_worker(tty);
 		tty_wakeup(tty->link);
 		return;

@@ -281,8 +279,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 		tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			break;
-		if (!tty->count)
-			break;
 		n_tty_kick_worker(tty);
 		unthrottled = tty_unthrottle_safe(tty);
 		if (!unthrottled)

Signature · 1 line

2.7.0

Peter Hurley

author

2016-01-10 05:46 UTC #19 lore raw

PATCH v2 7/7 tty: n_tty: fix SIGIO for output

In reply to Peter Hurley

n_tty_write() should set TTY_DO_WRITE_WAKEUP only when not all data could be written to the buffer.

[pjh: Also fixes missed SIGIO if amt written just happens to be
[ amount still to write

Signed-off-by Johannes Stezenbach <[email protected]>

[pjh: minor patch edits and re-submit]

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/tty/n_tty.c +1 -1

index 5ae661c..fad365a 100644
@@ -2361,7 +2361,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 	}
 break_out:
 	remove_wait_queue(&tty->write_wait, &wait);
-	if (b - buf != nr && tty->fasync)
+	if (nr && tty->fasync)
 		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	up_read(&tty->termios_rwsem);
 	return (b - buf) ? b - buf : retval;

Signature · 1 line

2.7.0