Re: [Libguestfs] [nbdkit PATCH] crypto: Tweak handling of SEND_MORE

In the recent commit 3842a080 to add SEND_MORE support, I blindly implemented the tls code as: if (SEND_MORE) { cork send } else { send uncork } because it showed improvements for my test case of aio-parallel-load from libnbd. But that test sticks to 64k I/O requests. But with further investigation, I've learned that even though gnutls corking works great for smaller NBD_CMD_READ replies, it can actually pessimize behavior for larger requests (such as a client sending lots of 2M read requests). This is because gnutls loses time both to realloc()s to copy the entire packet into memory, as well as CPU time spent encrypting the entire payload before anything is sent, not to mention that it still ends up fragmenting the message to fit TCP segment sizing. So, let's further tweak the logic to make a decision based on a heuristic: if we're going to split the reply for exceeding a TCP segment anyway, then uncork before the data (this sends the header out early as a partial packet, but that happens while the CPU is encrypting the large payload); otherwise, there's still hope that we can merge the previous request and this one into a single TCP segment (which may or may not happen, based on how much overhead TLS adds, given that 64k is the maximum TCP segment). It may turn out that we can tune the limit for slightly better performance (maybe 32k is smarter than 64k), but since the previous commit saw improvement with the benchmark using 64k packets, that's the value picked for now. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/crypto.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/server/crypto.c b/server/crypto.c index 3f3d96b..356f04f 100644 --- a/server/crypto.c +++ b/server/crypto.c @@ -345,6 +345,12 @@ crypto_recv (struct connection *conn, void *vbuf, size_t len) return 1; } +/* If this send()'s length is so large that it is going to require + * multiple TCP segments anyway, there's no need to try and merge it + * with any corked data from a previous send that used SEND_MORE. + */ +#define MAX_SEND_MORE_LEN (64 * 1024) + /* Write buffer to GnuTLS and either succeed completely * (returns 0) or fail (returns -1). flags is ignored for now. */ @@ -357,7 +363,11 @@ crypto_send (struct connection *conn, const void *vbuf, size_t len, int flags) assert (session != NULL); - if (flags & SEND_MORE) + if (len >= MAX_SEND_MORE_LEN) { + if (gnutls_record_uncork (session, GNUTLS_RECORD_WAIT) < 0) + return -1; + } + else if (flags & SEND_MORE) gnutls_record_cork (session); while (len > 0) {