[PATCH] io: bounce-buffer TLS writes to avoid nagle go-slow

[PATCH] io: bounce-buffer TLS writes to avoid nagle go-slow

[Daniel P. Berrangé]

The migration code caches vmstate/ram writes into an iovec and
flushes this every 128kb.

The QIOChannelTLS receives the iovec, but size GNUTLS cannot
accept iovec data, it iterates calling send for each element.

As a result of the migration data pattern, this results in
GNUTLS putting writes on the wire that alternate between about
4k and 30 bytes.

This is triggering the nagle algorithm on migration-test for
many of the TLS test cases, resulting in a "go slow" for I/O
that eventually hits the migration timeout configured by the
test.

Not every contributor reports seeing the "go slow" but for
those who do see it, it hits >= 95% of the time for at least
one of the migration TLS test cases run by 'make check'.

While we could disable the nagle algorithm (and multifd
channels already do this), that would just lead to lots of
small TCP packets hitting the wire which is not good for
throughput. The migration code flushes in batches of 128kb
because it wants large writes for high throughput.

The only way to achieve this in the TLS code is to bounce
buffer the writes in order to flatten the iovec. We cannot
fully flatten, however, since GNUTLS puts a cap on the max
TLS record size it is willing to send, which is negotiated
with the server, typically 16 kb out of the box.

This patch thus queries the max TLS record size and then
flattens the iovec into buffers of this size. If the
iovec only contains a single element, bounce buffering
is skipped to avoid the redundant copy.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlssession.c         | 10 ++++++++++
 include/crypto/tlssession.h |  2 ++
 include/io/channel-tls.h    |  2 ++
 io/channel-tls.c            | 35 ++++++++++++++++++++++++++++++-----
 io/trace-events             |  1 +
 5 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 314e3e96ba..cf4daf8544 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -653,6 +653,11 @@ qcrypto_tls_session_get_peer_name(QCryptoTLSSession *session)
     return NULL;
 }
 
+size_t qcrypto_tls_session_get_send_buffer(QCryptoTLSSession *session)
+{
+    return gnutls_record_get_max_size(session->handle);
+}
+
 
 #else /* ! CONFIG_GNUTLS */
 
@@ -757,4 +762,9 @@ qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess)
     return NULL;
 }
 
+size_t qcrypto_tls_session_get_send_buffer(QCryptoTLSSession *sess)
+{
+    return 1;
+}
+
 #endif
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 28e419681e..b29648a0a9 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -369,4 +369,6 @@ int qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess,
  */
 char *qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess);
 
+size_t qcrypto_tls_session_get_send_buffer(QCryptoTLSSession *sess);
+
 #endif /* QCRYPTO_TLSSESSION_H */
diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
index 7e9023570d..1c22a3fd07 100644
--- a/include/io/channel-tls.h
+++ b/include/io/channel-tls.h
@@ -50,6 +50,8 @@ struct QIOChannelTLS {
     QIOChannelShutdown shutdown;
     guint hs_ioc_tag;
     guint bye_ioc_tag;
+    char *send_buffer;
+    size_t send_buffer_len;
 };
 
 /**
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 31ec4d236d..ba2699786e 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -24,6 +24,7 @@
 #include "io/channel-tls.h"
 #include "trace.h"
 #include "qemu/atomic.h"
+#include "qemu/iov.h"
 
 
 static ssize_t qio_channel_tls_write_handler(const void *buf,
@@ -201,6 +202,12 @@ static gboolean qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
         } else {
             trace_qio_channel_tls_credentials_allow(ioc);
         }
+
+        ioc->send_buffer_len = qcrypto_tls_session_get_send_buffer(
+            ioc->session);
+        ioc->send_buffer = g_new0(char, ioc->send_buffer_len);
+        trace_qio_channel_tls_send_buffer_len(ioc, ioc->send_buffer_len);
+
         qio_task_complete(task);
         return TRUE;
     } else {
@@ -376,6 +383,7 @@ static void qio_channel_tls_finalize(Object *obj)
         g_clear_handle_id(&ioc->bye_ioc_tag, g_source_remove);
     }
 
+    g_free(ioc->send_buffer);
     object_unref(OBJECT(ioc->master));
     qcrypto_tls_session_free(ioc->session);
 }
@@ -446,13 +454,30 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc,
                                       Error **errp)
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
-    size_t i;
     ssize_t done = 0;
+    size_t tot = iov_size(iov, niov);
 
-    for (i = 0 ; i < niov ; i++) {
+    /* Skip bounce buffer in simple case */
+    if (niov == 1) {
         ssize_t ret = qcrypto_tls_session_write(tioc->session,
-                                                iov[i].iov_base,
-                                                iov[i].iov_len,
+                                                iov[0].iov_base,
+                                                iov[0].iov_len,
+                                                errp);
+        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+            return QIO_CHANNEL_ERR_BLOCK;
+        } else if (ret < 0) {
+            return -1;
+        }
+        return ret;
+    }
+
+    while (done < tot) {
+        size_t got = iov_to_buf(iov, niov, done,
+                                tioc->send_buffer,
+                                tioc->send_buffer_len);
+        ssize_t ret = qcrypto_tls_session_write(tioc->session,
+                                                tioc->send_buffer,
+                                                got,
                                                 errp);
         if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
             if (done) {
@@ -464,7 +489,7 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc,
             return -1;
         }
         done += ret;
-        if (ret < iov[i].iov_len) {
+        if (ret < got) {
             break;
         }
     }
diff --git a/io/trace-events b/io/trace-events
index ec91453335..88ebd5b478 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -44,6 +44,7 @@ qio_channel_tls_handshake_pending(void *ioc, int status) "TLS handshake pending
 qio_channel_tls_handshake_fail(void *ioc) "TLS handshake fail ioc=%p"
 qio_channel_tls_handshake_complete(void *ioc) "TLS handshake complete ioc=%p"
 qio_channel_tls_handshake_cancel(void *ioc) "TLS handshake cancel ioc=%p"
+qio_channel_tls_send_buffer_len(void *ioc, int len) "TLS send buffer len ioc=%p len=%d"
 qio_channel_tls_bye_start(void *ioc) "TLS termination start ioc=%p"
 qio_channel_tls_bye_pending(void *ioc, int status) "TLS termination pending ioc=%p status=%d"
 qio_channel_tls_bye_fail(void *ioc) "TLS termination fail ioc=%p"
-- 
2.55.0

Re: [PATCH] io: bounce-buffer TLS writes to avoid nagle go-slow

[Peter Xu]

On Mon, Sep 07, 2026 at 03:51:12PM +0100, Daniel P. Berrangé wrote:
> The migration code caches vmstate/ram writes into an iovec and
> flushes this every 128kb.

I am just curious how did this 128K came from. Perhaps this?

  MAX_IOV_SIZE / 2 * 4K

Where QEMU has:

#define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)

And it needs to divides 2 because we always push save_page_header() first,
which is 8B (in reality, maybe that'll also include some footers ahead from
the last page..), then another 4K following it.  Then in average when
hitting 64 io vectors there're 32 pages, coming up to be that.

I think that is right math for bulk ram phase, but maybe worth spelling out
a bit.. because if above holds it's not very obvious..

> 
> The QIOChannelTLS receives the iovec, but size GNUTLS cannot
> accept iovec data, it iterates calling send for each element.
> 
> As a result of the migration data pattern, this results in
> GNUTLS putting writes on the wire that alternate between about
> 4k and 30 bytes.
> 
> This is triggering the nagle algorithm on migration-test for
> many of the TLS test cases, resulting in a "go slow" for I/O
> that eventually hits the migration timeout configured by the
> test.

Worth spell out the qio_channel_set_delay() experiment?

Frankly, even knowing qio_channel_set_delay(NO_DELAY) on all channels would
fix it too, I don't think I fully get why the hang happened.

Nagle, if my understanding is correct.. should be something trying to
accumulate small writes only, it means write can be slightly delayed, but
it didn't further explain why even if we push writting to it, it didn't
flush properly.

Say, I understand TLS is special now with its io_writev(), being
qio_channel_tls_writev(), split the iov into multiple calls to
qcrypto_tls_session_write(), which is likely why the problem existed, but I
don't think I know why multiple qcrypto_tls_session_write() (and I believe
ultimately, assuming small but continuous write()s to the socket fd) will
cause a hang.  Any clue?

> 
> Not every contributor reports seeing the "go slow" but for
> those who do see it, it hits >= 95% of the time for at least
> one of the migration TLS test cases run by 'make check'.
> 
> While we could disable the nagle algorithm (and multifd
> channels already do this), that would just lead to lots of
> small TCP packets hitting the wire which is not good for
> throughput. The migration code flushes in batches of 128kb
> because it wants large writes for high throughput.
> 
> The only way to achieve this in the TLS code is to bounce
> buffer the writes in order to flatten the iovec. We cannot
> fully flatten, however, since GNUTLS puts a cap on the max
> TLS record size it is willing to send, which is negotiated
> with the server, typically 16 kb out of the box.
> 
> This patch thus queries the max TLS record size and then
> flattens the iovec into buffers of this size. If the
> iovec only contains a single element, bounce buffering
> is skipped to avoid the redundant copy.

I saw there is also gnutls_record_cork() and the uncork(), which seems to
resolve the same issue (I tried to look at gnutls git history but I didn't
find any mention of why the API introduced.. though).

Any thoughts on why not relying on that, say, would it work too if cork()
at start of qio_channel_tls_writev(), loop, then uncork()?

Thanks,

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  crypto/tlssession.c         | 10 ++++++++++
>  include/crypto/tlssession.h |  2 ++
>  include/io/channel-tls.h    |  2 ++
>  io/channel-tls.c            | 35 ++++++++++++++++++++++++++++++-----
>  io/trace-events             |  1 +
>  5 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 314e3e96ba..cf4daf8544 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -653,6 +653,11 @@ qcrypto_tls_session_get_peer_name(QCryptoTLSSession *session)
>      return NULL;
>  }
>  
> +size_t qcrypto_tls_session_get_send_buffer(QCryptoTLSSession *session)
> +{
> +    return gnutls_record_get_max_size(session->handle);
> +}
> +
>  
>  #else /* ! CONFIG_GNUTLS */
>  
> @@ -757,4 +762,9 @@ qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess)
>      return NULL;
>  }
>  
> +size_t qcrypto_tls_session_get_send_buffer(QCryptoTLSSession *sess)
> +{
> +    return 1;
> +}
> +
>  #endif
> diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> index 28e419681e..b29648a0a9 100644
> --- a/include/crypto/tlssession.h
> +++ b/include/crypto/tlssession.h
> @@ -369,4 +369,6 @@ int qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess,
>   */
>  char *qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess);
>  
> +size_t qcrypto_tls_session_get_send_buffer(QCryptoTLSSession *sess);
> +
>  #endif /* QCRYPTO_TLSSESSION_H */
> diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
> index 7e9023570d..1c22a3fd07 100644
> --- a/include/io/channel-tls.h
> +++ b/include/io/channel-tls.h
> @@ -50,6 +50,8 @@ struct QIOChannelTLS {
>      QIOChannelShutdown shutdown;
>      guint hs_ioc_tag;
>      guint bye_ioc_tag;
> +    char *send_buffer;
> +    size_t send_buffer_len;
>  };
>  
>  /**
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 31ec4d236d..ba2699786e 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -24,6 +24,7 @@
>  #include "io/channel-tls.h"
>  #include "trace.h"
>  #include "qemu/atomic.h"
> +#include "qemu/iov.h"
>  
>  
>  static ssize_t qio_channel_tls_write_handler(const void *buf,
> @@ -201,6 +202,12 @@ static gboolean qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
>          } else {
>              trace_qio_channel_tls_credentials_allow(ioc);
>          }
> +
> +        ioc->send_buffer_len = qcrypto_tls_session_get_send_buffer(
> +            ioc->session);
> +        ioc->send_buffer = g_new0(char, ioc->send_buffer_len);
> +        trace_qio_channel_tls_send_buffer_len(ioc, ioc->send_buffer_len);
> +
>          qio_task_complete(task);
>          return TRUE;
>      } else {
> @@ -376,6 +383,7 @@ static void qio_channel_tls_finalize(Object *obj)
>          g_clear_handle_id(&ioc->bye_ioc_tag, g_source_remove);
>      }
>  
> +    g_free(ioc->send_buffer);
>      object_unref(OBJECT(ioc->master));
>      qcrypto_tls_session_free(ioc->session);
>  }
> @@ -446,13 +454,30 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc,
>                                        Error **errp)
>  {
>      QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> -    size_t i;
>      ssize_t done = 0;
> +    size_t tot = iov_size(iov, niov);
>  
> -    for (i = 0 ; i < niov ; i++) {
> +    /* Skip bounce buffer in simple case */
> +    if (niov == 1) {
>          ssize_t ret = qcrypto_tls_session_write(tioc->session,
> -                                                iov[i].iov_base,
> -                                                iov[i].iov_len,
> +                                                iov[0].iov_base,
> +                                                iov[0].iov_len,
> +                                                errp);
> +        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
> +            return QIO_CHANNEL_ERR_BLOCK;
> +        } else if (ret < 0) {
> +            return -1;
> +        }
> +        return ret;
> +    }
> +
> +    while (done < tot) {
> +        size_t got = iov_to_buf(iov, niov, done,
> +                                tioc->send_buffer,
> +                                tioc->send_buffer_len);
> +        ssize_t ret = qcrypto_tls_session_write(tioc->session,
> +                                                tioc->send_buffer,
> +                                                got,
>                                                  errp);
>          if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
>              if (done) {
> @@ -464,7 +489,7 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc,
>              return -1;
>          }
>          done += ret;
> -        if (ret < iov[i].iov_len) {
> +        if (ret < got) {
>              break;
>          }
>      }
> diff --git a/io/trace-events b/io/trace-events
> index ec91453335..88ebd5b478 100644
> --- a/io/trace-events
> +++ b/io/trace-events
> @@ -44,6 +44,7 @@ qio_channel_tls_handshake_pending(void *ioc, int status) "TLS handshake pending
>  qio_channel_tls_handshake_fail(void *ioc) "TLS handshake fail ioc=%p"
>  qio_channel_tls_handshake_complete(void *ioc) "TLS handshake complete ioc=%p"
>  qio_channel_tls_handshake_cancel(void *ioc) "TLS handshake cancel ioc=%p"
> +qio_channel_tls_send_buffer_len(void *ioc, int len) "TLS send buffer len ioc=%p len=%d"
>  qio_channel_tls_bye_start(void *ioc) "TLS termination start ioc=%p"
>  qio_channel_tls_bye_pending(void *ioc, int status) "TLS termination pending ioc=%p status=%d"
>  qio_channel_tls_bye_fail(void *ioc) "TLS termination fail ioc=%p"
> -- 
> 2.55.0
> 

-- 
Peter Xu

Re: [PATCH] io: bounce-buffer TLS writes to avoid nagle go-slow

[Daniel P. Berrangé]

On Tue, Sep 08, 2026 at 02:12:53PM -0400, Peter Xu wrote:
> On Mon, Sep 07, 2026 at 03:51:12PM +0100, Daniel P. Berrangé wrote:
> > The migration code caches vmstate/ram writes into an iovec and
> > flushes this every 128kb.
> 
> I am just curious how did this 128K came from. Perhaps this?
> 
>   MAX_IOV_SIZE / 2 * 4K
> 
> Where QEMU has:
> 
> #define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
> 
> And it needs to divides 2 because we always push save_page_header() first,
> which is 8B (in reality, maybe that'll also include some footers ahead from
> the last page..), then another 4K following it.  Then in average when
> hitting 64 io vectors there're 32 pages, coming up to be that.
> 
> I think that is right math for bulk ram phase, but maybe worth spelling out
> a bit.. because if above holds it's not very obvious..

Tracing the qio_channel_writev() calls yet again, I think my
mention of 128k is wrong. I'm now actually seeing alot of
1/2 MB writes. eg

Writev 64 (nvio=1)
Writev 64 (nvio=1)
Writev 64 (nvio=1)
Writev 64 (nvio=1)
Writev 1344 (nvio=1)
Writev 1344 (nvio=1)
Writev 1344 (nvio=1)
Writev 1344 (nvio=1)
Writev 281 (nvio=1)
Writev 8 (nvio=1)
Writev 525632 (nvio=129)
Writev 525632 (nvio=129)
Writev 13632 (nvio=4)
Writev 173376 (nvio=43)
Writev 525632 (nvio=129)
Writev 525632 (nvio=129)
Writev 525632 (nvio=129)
Writev 525632 (nvio=129)
Writev 525632 (nvio=129)
Writev 525632 (nvio=129)


> 
> > 
> > The QIOChannelTLS receives the iovec, but size GNUTLS cannot
> > accept iovec data, it iterates calling send for each element.
> > 
> > As a result of the migration data pattern, this results in
> > GNUTLS putting writes on the wire that alternate between about
> > 4k and 30 bytes.
> > 
> > This is triggering the nagle algorithm on migration-test for
> > many of the TLS test cases, resulting in a "go slow" for I/O
> > that eventually hits the migration timeout configured by the
> > test.
> 
> Worth spell out the qio_channel_set_delay() experiment?
> 
> Frankly, even knowing qio_channel_set_delay(NO_DELAY) on all channels would
> fix it too, I don't think I fully get why the hang happened.

Note, it was never technically a "hang", it was just a "go slow".
The src was still sending and the dst was still receiving but it
was pathologically slow, a few KBs per second, instead of 100s or
1000s of MBs.

> Nagle, if my understanding is correct.. should be something trying to
> accumulate small writes only, it means write can be slightly delayed, but
> it didn't further explain why even if we push writting to it, it didn't
> flush properly.

The nagle algorithm influences the TCP window size. The src cannot
send more data, until the dst has acknowledged packets already sent.

IIUC, normally if you send large volumes of data the window size will
grow large quite quickly. If you send lots of small packets, nagle
can keep the window size small and thus delay pending writes.

Migration with large iovec arrays was causnig alot of small writes,
so I think that meant the window size did not grow enough to get
a high speed.

> Say, I understand TLS is special now with its io_writev(), being
> qio_channel_tls_writev(), split the iov into multiple calls to
> qcrypto_tls_session_write(), which is likely why the problem existed, but I
> don't think I know why multiple qcrypto_tls_session_write() (and I believe
> ultimately, assuming small but continuous write()s to the socket fd) will
> cause a hang.  Any clue?

What I can't explain is why only certain contributors ever saw this
as a problem ?

> > This patch thus queries the max TLS record size and then
> > flattens the iovec into buffers of this size. If the
> > iovec only contains a single element, bounce buffering
> > is skipped to avoid the redundant copy.
> 
> I saw there is also gnutls_record_cork() and the uncork(), which seems to
> resolve the same issue (I tried to look at gnutls git history but I didn't
> find any mention of why the API introduced.. though).
> 
> Any thoughts on why not relying on that, say, would it work too if cork()
> at start of qio_channel_tls_writev(), loop, then uncork()?

Yes, relying on gnutls_record_cork is something I can try - it would
certainly be nice to avoid the bounce buffering, as that's significant
overhead when we're talking about iovecs with 1/2 MB of data at a
time.

I'll prepare a v2.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] io: bounce-buffer TLS writes to avoid nagle go-slow

[Peter Xu]

On Tue, Sep 08, 2026 at 08:24:56PM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2026 at 02:12:53PM -0400, Peter Xu wrote:
> > On Mon, Sep 07, 2026 at 03:51:12PM +0100, Daniel P. Berrangé wrote:
> > > The migration code caches vmstate/ram writes into an iovec and
> > > flushes this every 128kb.
> > 
> > I am just curious how did this 128K came from. Perhaps this?
> > 
> >   MAX_IOV_SIZE / 2 * 4K
> > 
> > Where QEMU has:
> > 
> > #define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
> > 
> > And it needs to divides 2 because we always push save_page_header() first,
> > which is 8B (in reality, maybe that'll also include some footers ahead from
> > the last page..), then another 4K following it.  Then in average when
> > hitting 64 io vectors there're 32 pages, coming up to be that.
> > 
> > I think that is right math for bulk ram phase, but maybe worth spelling out
> > a bit.. because if above holds it's not very obvious..
> 
> Tracing the qio_channel_writev() calls yet again, I think my
> mention of 128k is wrong. I'm now actually seeing alot of
> 1/2 MB writes. eg

The 1/2 MB writes are likely from multifd senders.

> 
> Writev 64 (nvio=1)
> Writev 64 (nvio=1)
> Writev 64 (nvio=1)
> Writev 64 (nvio=1)

These are likely, MultiFDInit_t, and maybe there're just 4 multifd
channels?

> Writev 1344 (nvio=1)
> Writev 1344 (nvio=1)
> Writev 1344 (nvio=1)
> Writev 1344 (nvio=1)
> Writev 281 (nvio=1)
> Writev 8 (nvio=1)
> Writev 525632 (nvio=129)
> Writev 525632 (nvio=129)
> Writev 13632 (nvio=4)
> Writev 173376 (nvio=43)
> Writev 525632 (nvio=129)
> Writev 525632 (nvio=129)
> Writev 525632 (nvio=129)
> Writev 525632 (nvio=129)
> Writev 525632 (nvio=129)
> Writev 525632 (nvio=129)
> 
> 
> > 
> > > 
> > > The QIOChannelTLS receives the iovec, but size GNUTLS cannot
> > > accept iovec data, it iterates calling send for each element.
> > > 
> > > As a result of the migration data pattern, this results in
> > > GNUTLS putting writes on the wire that alternate between about
> > > 4k and 30 bytes.
> > > 
> > > This is triggering the nagle algorithm on migration-test for
> > > many of the TLS test cases, resulting in a "go slow" for I/O
> > > that eventually hits the migration timeout configured by the
> > > test.
> > 
> > Worth spell out the qio_channel_set_delay() experiment?
> > 
> > Frankly, even knowing qio_channel_set_delay(NO_DELAY) on all channels would
> > fix it too, I don't think I fully get why the hang happened.
> 
> Note, it was never technically a "hang", it was just a "go slow".
> The src was still sending and the dst was still receiving but it
> was pathologically slow, a few KBs per second, instead of 100s or
> 1000s of MBs.

Ah OK, yes "hang" isn't accurate.  IMHO it would be nice to mention the
bandwidth measured in the commit log.

> 
> > Nagle, if my understanding is correct.. should be something trying to
> > accumulate small writes only, it means write can be slightly delayed, but
> > it didn't further explain why even if we push writting to it, it didn't
> > flush properly.
> 
> The nagle algorithm influences the TCP window size. The src cannot
> send more data, until the dst has acknowledged packets already sent.

OK, so it's TLS specific behavior (within gnutls)?

> 
> IIUC, normally if you send large volumes of data the window size will
> grow large quite quickly. If you send lots of small packets, nagle
> can keep the window size small and thus delay pending writes.
> 
> Migration with large iovec arrays was causnig alot of small writes,
> so I think that meant the window size did not grow enough to get
> a high speed.
> 
> > Say, I understand TLS is special now with its io_writev(), being
> > qio_channel_tls_writev(), split the iov into multiple calls to
> > qcrypto_tls_session_write(), which is likely why the problem existed, but I
> > don't think I know why multiple qcrypto_tls_session_write() (and I believe
> > ultimately, assuming small but continuous write()s to the socket fd) will
> > cause a hang.  Any clue?
> 
> What I can't explain is why only certain contributors ever saw this
> as a problem ?

Me too.  I think the NODELAY test at least proved it is relevant to how ACK
happens, and if that ACK delay behaves differently on different host, it
may explain.

> 
> > > This patch thus queries the max TLS record size and then
> > > flattens the iovec into buffers of this size. If the
> > > iovec only contains a single element, bounce buffering
> > > is skipped to avoid the redundant copy.
> > 
> > I saw there is also gnutls_record_cork() and the uncork(), which seems to
> > resolve the same issue (I tried to look at gnutls git history but I didn't
> > find any mention of why the API introduced.. though).
> > 
> > Any thoughts on why not relying on that, say, would it work too if cork()
> > at start of qio_channel_tls_writev(), loop, then uncork()?
> 
> Yes, relying on gnutls_record_cork is something I can try - it would
> certainly be nice to avoid the bounce buffering, as that's significant
> overhead when we're talking about iovecs with 1/2 MB of data at a
> time.

I had a quick look at v2, when looking into the cork() a bit more, I found
that gnutls is doing the caching before encryption not after, so I think
there's still a bounce buffer..

Said that, I wonder if using cork() is still a good approach, not only if
that solves the current problem, but also because it trades "memcpy" with
"less syscalls" too as side effect: IIUC we used to write() too frequently,
in case of RAM headers maybe one write on a few bytes worst case, but now
it's one shot, and IIUC the size should be the same as qemufile caching.

What I plan to do is I want to do a simple perf test tomorrow with TLS
migration, single threaded as start, to see if v2 would improve performance
(ignoring the fact it would fix the nodelay issue).

Another thing I can report early is v2 fails to compile when gnutls-devel
isn't available.

Thanks,

> 
> I'll prepare a v2.
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> |: https://libvirt.org          ~~          https://entangle-photo.org :|
> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
> 

-- 
Peter Xu