Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290473 > unrolled thread

[PATCH 0/6] More n_tty fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] More n_tty fixes Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
    [PATCH 6/6] n_tty: Remove tty count checks from unthrottle Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
    [PATCH 4/6] tty: Fix ioctl(FIOASYNC) on hungup file Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
    [PATCH 3/6] tty: Add fasync() hung up file operation Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
    [PATCH 1/6] n_tty: Always wake up read()/poll() if new input Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
      Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input Johannes Stezenbach <js@sig21.net> - 2015-12-13 15:50 +0100
        Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input Peter Hurley <peter@hurleysoftware.com> - 2015-12-13 21:00 +0100
    [PATCH 2/6] tty, n_tty: Remove fasync() ldisc notification Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100

#1290473 — [PATCH 0/6] 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(-)

-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1290474 — [PATCH 6/6] n_tty: Remove tty count checks from unthrottle

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 <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 5034da0..dd34c5d 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -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)
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290475 — [PATCH 4/6] tty: Fix ioctl(FIOASYNC) on hungup file

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 <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e9b7591..19cec0e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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;
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290476 — [PATCH 3/6] tty: Add fasync() hung up file operation

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 <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index df7a3a9..e9b7591 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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);
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290477 — [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 <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 33 ++-------------------------------
 1 file changed, 2 insertions(+), 31 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4e1867a..16ba07b 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -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)
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Hi Peter,

On Sat, Dec 12, 2015 at 02:16:34PM -0800, Peter Hurley wrote:
> 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).
...
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Hi Johannes,

On 12/13/2015 06:49 AM, Johannes Stezenbach wrote:
> Hi Peter,
> 
> On Sat, Dec 12, 2015 at 02:16:34PM -0800, Peter Hurley wrote:
>> 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).
> ...
>> @@ -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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290478 — [PATCH 2/6] tty, n_tty: Remove fasync() ldisc notification

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 <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c       | 5 -----
 drivers/tty/tty_io.c      | 8 --------
 include/linux/tty_ldisc.h | 6 ------
 3 files changed, 19 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 16ba07b..e695f8f 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -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,
 };
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5c8f519..df7a3a9 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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;
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index db0abe56..08fd06a 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -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);
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web