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

[Richard W.M. Jones]

On Mon, Jun 10, 2019 at 05:11:26PM -0500, Eric Blake wrote:
> 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 at redhat.com>
> ---
>  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) {

Looks reasonable, ACK.

In answer to the previous question about how to choose the threshold,
I have no idea either :-(

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html