On Tue, 9 Nov 2021, rapier wrote:
So this isn't an issue as much as a weird situation I am not fully
understanding. That said, if I can understand it then it might be a benefit.
In the function client_process_net_input in clientloop.c if I increase the
size of buf[SSH_IOBUFSZ] to 128k I'm seeing a pretty substantial performance
improvement - mostly when using aes256-ctr.
For example; with the command
./ssh HostB -caes256-ctr "cat /dev/zero" > /dev/null
I'm seeing throughput of around 610MB/s on a 10Gb network.
When I use an unmodified version I'll see 480MB/s.
Same hosts, same command. The only difference being the size of buf in
client_process_net_input.
HostA is a Xeon x5675 @3Ghz. HostB is an AMD Ryzen 7 5800X.
My initial assumption is since HostA is CPU bound reducing the number of reads
has a significant impact. That said, a nearly 30% improvement seems excessive
for that to be all that's going on. Additionally, I'm not seeing as much
improvement using chachapoly. In that case, I'm only seeing about a 20%
improvement. 310MB/s for stock and 375MB/s for the big buffer.
Additionally, I'm only seeing the improvement when HostB is sending the data
and HostA receiving. When HostA (the Xeon) is sending (cat /dev/zero | ./ssh
HostB "cat > /dev/null") then I'm seeing about 290MB/s with either version.
I'm not suggesting any changes to the code. I'm just trying to understand what
might be happening as it could be the buffer size, something with the CPU
architecture, the switch I'm using, the distro (HostA is fedora, HostB is
ubuntu), etc. Any clues would be appreciated.
Maybe try this instead, it avoids the intermediate buffer and tries to make
larger reads directly to the channel buffer. I wrote it while trying to
investigate a report that re-enabling TCP Nagle improved performance,
but I couldn't replicate the reported problem.
(I wrote this a while ago, but it's only lightly tested)
diff --git a/channels.c b/channels.c
index 4903ad1..d6b2024 100644
--- a/channels.c
+++ b/channels.c
@@ -1928,16 +1928,41 @@ channel_handle_rfd(struct ssh *ssh, Channel *c,
char buf[CHAN_RBUF];
ssize_t len;
int r;
+ size_t avail, rlen, maxlen;
if (c->rfd == -1 || !FD_ISSET(c->rfd, readset))
return 1;
+ /*
+ * For "simple" channels (i.e. not datagram or filtered), try to
+ * read up to a complete remote window of data directly to the
+ * channel buffer.
+ */
+ if (c->input_filter == NULL && !c->datagram) {
+ if (sshbuf_len(c->input) >= c->remote_window ||
+ (avail = sshbuf_avail(c->input)) == 0)
+ return 1; /* shouldn't happen */
+ maxlen = c->remote_window - sshbuf_len(c->input);
+ if (maxlen > avail)
+ maxlen = avail;
+ if (maxlen > CHANNEL_MAX_READ)
+ maxlen = CHANNEL_MAX_READ;
+ if ((r = sshbuf_read(c->rfd, c->input, maxlen, &rlen)) != 0) {
+ debug2("channel %d: read failed rfd %d maxlen %zu: %s",
+ c->self, c->rfd, maxlen, ssh_err(r));
+ goto rfail;
+ }
+ return 1;
+ }
+
len = read(c->rfd, buf, sizeof(buf));
if (len == -1 && (errno == EINTR || errno == EAGAIN))
return 1;
if (len <= 0) {
- debug2("channel %d: read<=0 rfd %d len %zd",
- c->self, c->rfd, len);
+ debug2("channel %d: read<=0 rfd %d len %zd: %s",
+ c->self, c->rfd, len,
+ len == 0 ? "closed" : strerror(errno));
+ rfail:
if (c->type != SSH_CHANNEL_OPEN) {
debug2("channel %d: not open", c->self);
chan_mark_dead(ssh, c);
@@ -1955,8 +1980,7 @@ channel_handle_rfd(struct ssh *ssh, Channel *c,
} else if (c->datagram) {
if ((r = sshbuf_put_string(c->input, buf, len)) != 0)
fatal_fr(r, "channel %i: put datagram", c->self);
- } else if ((r = sshbuf_put(c->input, buf, len)) != 0)
- fatal_fr(r, "channel %i: put data", c->self);
+ }
return 1;
}
diff --git a/channels.h b/channels.h
index 633adc3..db90c18 100644
--- a/channels.h
+++ b/channels.h
@@ -231,6 +231,9 @@ struct Channel {
/* Read buffer size */
#define CHAN_RBUF (16*1024)
+/* Maximum size for direct reads to buffers */
+#define CHANNEL_MAX_READ (CHAN_SES_WINDOW_DEFAULT*2)
+
/* Maximum channel input buffer size */
#define CHAN_INPUT_MAX (16*1024*1024)
diff --git a/sshbuf-io.c b/sshbuf-io.c
index 966f820..0b7628f 100644
--- a/sshbuf-io.c
+++ b/sshbuf-io.c
@@ -113,3 +113,39 @@ sshbuf_write_file(const char *path, struct sshbuf *buf)
return 0;
}
+int
+sshbuf_read(int fd, struct sshbuf *buf, size_t maxlen, size_t *rlen)
+{
+ int r, oerrno;
+ size_t adjust;
+ ssize_t rr;
+ u_char *d;
+
+ if (rlen != NULL)
+ *rlen = 0;
+ if ((r = sshbuf_reserve(buf, maxlen, &d)) != 0)
+ return r;
+ rr = read(fd, d, maxlen);
+ oerrno = errno;
+
+ /* Adjust the buffer to include only what was actually read */
+ if (rr > 0 && (adjust = maxlen - rr) > 0) {
+ if ((r = sshbuf_consume_end(buf, adjust)) != 0) {
+ /* avoid returning uninitialised data to caller */
+ memset(d + rr, '\0', adjust);
+ return SSH_ERR_INTERNAL_ERROR; /* shouldn't happen */
+ }
+ }
+ if (rr < 0) {
+ errno = oerrno;
+ return SSH_ERR_SYSTEM_ERROR;
+ } else if (rr == 0) {
+ errno = EPIPE;
+ return SSH_ERR_SYSTEM_ERROR;
+ }
+ /* success */
+ if (rlen != NULL)
+ *rlen = (size_t)rr;
+ return 0;
+}
+
diff --git a/sshbuf.h b/sshbuf.h
index 2b77d15..1ee9266 100644
--- a/sshbuf.h
+++ b/sshbuf.h
@@ -310,6 +310,10 @@ int sshbuf_load_file(const char *, struct sshbuf **)
int sshbuf_write_file(const char *path, struct sshbuf *buf)
__attribute__((__nonnull__ (2)));
+/* Read up to maxlen bytes from a fd directly to a buffer */
+int sshbuf_read(int, struct sshbuf *, size_t, size_t *)
+ __attribute__((__nonnull__ (2)));
+
/* Macros for decoding/encoding integers */
#define PEEK_U64(p) \
(((u_int64_t)(((const u_char *)(p))[0]) << 56) | \
