On Tue, Jan 19, 2021 at 5:29 PM Oliver Giles <ohw.gi...@gmail.com> wrote:
>
> Writing this from a kernel with those patches in; happily splice()ing
> to and from a pty.
Ok, good.
I have a couple of improvement patches on top of those, that I'm attaching here.
The first four patches worked fine for me (and apparently for you
too), but due to the small buffer, the regular N_TTY case for tty
read() calls are now limited to 64 bytes each.
That is unfortunate for performance, but it might also cause some
actual breakage: anybody doing "read()" calls on a tty file descriptor
_should_ be perfectly fine with short reads since they happen for
signals etc, but I could well imagine that not everybody is.
And that new kernel buffer interface was _designed_ to allow stitching
small buffers together efficiently (since the hdlc case needed it), so
this implements that for the non-icanon case for n_tty too.
I wasted an embarrasing amount of time today on that final patch - I
spent something like 6 hours chasing a truly stupid one-liner bug I
had initially, and couldn't see what was wrong.
Which is why this only does the non-icanon case, because after I
finally had my "Duh!" moment, I was so annoyed with it that I had to
just walk away.
I'll come back to this tomorrow and do the line-buffered icanon case
too (unless pull requests pile up), and then I'll be happy with the
tty changes, and I think I can submit this series for real to Greg.
But in the meantime, here's two more patches to try on top of the
previous four. They shouldn't matter, other than making the non-icanon
throughput a lot better. But the more coverage they get, the happier
I'll be.
Linus
From b12a1652c91648e96ae11946f7489515dd063789 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torva...@linux-foundation.org>
Date: Tue, 19 Jan 2021 13:46:28 -0800
Subject: [PATCH 5/6] tty: clean up legacy leftovers from n_tty line discipline
Back when the line disciplines did their own direct user accesses, they
had to deal with the data copy possibly failing in the middle.
Now that the user copy is done by the tty_io.c code, that failure case
no longer exists.
Remove the left-over error handling code that cannot trigger.
Signed-off-by: Linus Torvalds <torva...@linux-foundation.org>
---
drivers/tty/n_tty.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2f2f57a53968..a02fe661f617 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1957,19 +1957,17 @@ static inline int input_available_p(struct tty_struct *tty, int poll)
* read_tail published
*/
-static int copy_from_read_buf(struct tty_struct *tty,
+static void copy_from_read_buf(struct tty_struct *tty,
unsigned char **kbp,
size_t *nr)
{
struct n_tty_data *ldata = tty->disc_data;
- int retval;
size_t n;
bool is_eof;
size_t head = smp_load_acquire(&ldata->commit_head);
size_t tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1);
- retval = 0;
n = min(head - ldata->read_tail, N_TTY_BUF_SIZE - tail);
n = min(*nr, n);
if (n) {
@@ -1986,7 +1984,6 @@ static int copy_from_read_buf(struct tty_struct *tty,
*kbp += n;
*nr -= n;
}
- return retval;
}
/**
@@ -2228,20 +2225,15 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
if (retval)
break;
} else {
- int uncopied;
-
/* Deal with packet mode. */
if (packet && kb == kbuf) {
*kb++ = TIOCPKT_DATA;
nr--;
}
- uncopied = copy_from_read_buf(tty, &kb, &nr);
- uncopied += copy_from_read_buf(tty, &kb, &nr);
- if (uncopied) {
- retval = -EFAULT;
- break;
- }
+ /* See comment above copy_from_read_buf() why twice */
+ copy_from_read_buf(tty, &kb, &nr);
+ copy_from_read_buf(tty, &kb, &nr);
}
n_tty_check_unthrottle(tty);
--
2.29.2.157.g1d47791a39
From e724cc9c4b101a4de1a56bcca6b5ec1d3493b173 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torva...@linux-foundation.org>
Date: Tue, 19 Jan 2021 18:14:20 -0800
Subject: [PATCH 6/6] tty: teach n_tty line discipline about the new "cookie
continuations"
With the conversion to do the tty ldisc read operations in small chunks,
the n_tty line discipline became noticeably slower for throughput
oriented loads, because rather than read things in up to 2kB chunks, it
would return at most 64 bytes per read() system call.
The cost is mainly all in the "do system calls over and over", not
really in the new "copy to an extra kernel buffer".
This can be fixed by teaching the n_tty line discipline about the
"cookie continuation" model, which the chunking code supports because
things like hdlc need to be able to handle packets up to 64kB in size.
Doing that doesn't just get us back to the old performace, but to much
better performance: my stupid "copy 10MB of data over a pty" test
program is now almost twice as fast as it used to be (going down from
0.1s to 0.054s).
This is entirely because it now creates maximal chunks (which happens to
be "one byte less than one page" due to how we do the circular tty
buffers).
NOTE! This case only handles the simpler non-icanon case, which is the
one where people may care about throughput. I'm going to do the icanon
case later too, because while performance isn't a major issue for that,
there may be programs that think they'll always get a full line and
don't like the 64-byte chunking for that reason.
Such programs are arguably buggy (signals etc can cause random partial
results from tty reads anyway), and good programs will handle such
partial reads, but expecting everybody to write "good programs" has
never been a winning policy for the kernel..
Signed-off-by: Linus Torvalds <torva...@linux-foundation.org>
---
drivers/tty/n_tty.c | 52 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index a02fe661f617..37bfd695011d 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1945,19 +1945,17 @@ static inline int input_available_p(struct tty_struct *tty, int poll)
* Helper function to speed up n_tty_read. It is only called when
* ICANON is off; it copies characters straight from the tty queue.
*
- * It can be profitably called twice; once to drain the space from
- * the tail pointer to the (physical) end of the buffer, and once
- * to drain the space from the (physical) beginning of the buffer
- * to head pointer.
- *
* Called under the ldata->atomic_read_lock sem
*
+ * Returns true if it successfully copied data, but there is still
+ * more data to be had.
+ *
* n_tty_read()/consumer path:
* caller holds non-exclusive termios_rwsem
* read_tail published
*/
-static void copy_from_read_buf(struct tty_struct *tty,
+static bool copy_from_read_buf(struct tty_struct *tty,
unsigned char **kbp,
size_t *nr)
@@ -1980,10 +1978,14 @@ static void copy_from_read_buf(struct tty_struct *tty,
/* Turn single EOF into zero-length read */
if (L_EXTPROC(tty) && ldata->icanon && is_eof &&
(head == ldata->read_tail))
- n = 0;
+ return false;
*kbp += n;
*nr -= n;
+
+ /* If we have more to copy, let the caller know */
+ return head != ldata->read_tail;
}
+ return false;
}
/**
@@ -2135,6 +2137,25 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
int packet;
size_t tail;
+ /*
+ * Is this a continuation of a read started earler?
+ *
+ * If so, we still hold the atomic_read_lock and the
+ * termios_rwsem, and can just continue to copy data.
+ */
+ if (*cookie) {
+ if (copy_from_read_buf(tty, &kb, &nr))
+ return kb - kbuf;
+
+ /* No more data - release locks and stop retries */
+ n_tty_kick_worker(tty);
+ n_tty_check_unthrottle(tty);
+ up_read(&tty->termios_rwsem);
+ mutex_unlock(&ldata->atomic_read_lock);
+ *cookie = NULL;
+ return kb - kbuf;
+ }
+
c = job_control(tty, file);
if (c < 0)
return c;
@@ -2231,9 +2252,20 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
nr--;
}
- /* See comment above copy_from_read_buf() why twice */
- copy_from_read_buf(tty, &kb, &nr);
- copy_from_read_buf(tty, &kb, &nr);
+ /*
+ * Copy data, and if there is more to be had
+ * and we have nothing more to wait for, then
+ * let's mark us for retries.
+ *
+ * NOTE! We return here with both the termios_sem
+ * and atomic_read_lock still held, the retries
+ * will release them when done.
+ */
+ if (copy_from_read_buf(tty, &kb, &nr) && kb - kbuf >= minimum) {
+ remove_wait_queue(&tty->read_wait, &wait);
+ *cookie = cookie;
+ return kb - kbuf;
+ }
}
n_tty_check_unthrottle(tty);
--
2.29.2.157.g1d47791a39
