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) | \