The migration code caches vmstate/ram writes into an iovec and flushes this periodically. The total amount of data to sent may be 100's of KB, but split across many iovec, each of which is potentially quite small.
The QIOChannelTLS receives the iovec, but since 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 lots of small TLS records on the wire. This has shown writes alternate between about 4k and 30 bytes in some tests. 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 stil result in lots of small TLS records hitting the wire which is not good for throughput. The migration code flushes in batches because it wants large writes for high throughput. To achieve this we must tell GNUTLS to encrypt data but not immediately sent TLS records by "corking" its output. Once the complete iovec has been written to GNUTLS, it can be uncorked allowing it to hit the wire. There is added complexity with uncorking on non-blocking channels as not all encrypted data can be sent at once. qio_channel_write() will return what was sent, but some data might remain pending inside GNUTLS buffers. A later call to qio_channel_write() will provide the same plain text data buffers that have already been cached. Thus the code must attempt to uncork GNUTLS again to clear pending data and then deduct the equivalent amount of plain text. Signed-off-by: Daniel P. Berrangé <[email protected]> --- crypto/tlssession.c | 33 ++++++++++++++ include/crypto/tlssession.h | 25 +++++++++++ include/io/channel-tls.h | 1 + io/channel-tls.c | 87 ++++++++++++++++++++++++++++++++++--- 4 files changed, 140 insertions(+), 6 deletions(-) diff --git a/crypto/tlssession.c b/crypto/tlssession.c index 314e3e96ba..ffb6a27e47 100644 --- a/crypto/tlssession.c +++ b/crypto/tlssession.c @@ -513,6 +513,39 @@ qcrypto_tls_session_read(QCryptoTLSSession *session, } +void qcrypto_tls_session_write_cork(QCryptoTLSSession *sess) +{ + gnutls_record_cork(sess->handle); +} + + +ssize_t qcrypto_tls_session_write_uncork(QCryptoTLSSession *sess, + Error **errp) +{ + int ret; + ret = gnutls_record_uncork(sess->handle, 0); + if (ret == GNUTLS_E_AGAIN || + ret == GNUTLS_E_INTERRUPTED) { + ret = gnutls_record_check_corked(sess->handle); + if (ret < 0) { + error_setg(errp, + "Cannot query pending TLS output: %s", + gnutls_strerror(ret)); + return -1; + } + + return ret; + } else if (ret < 0) { + error_setg(errp, + "Cannot uncork TLS output: %s", + gnutls_strerror(ret)); + return -1; + } + + return 0; +} + + size_t qcrypto_tls_session_check_pending(QCryptoTLSSession *session) { diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h index 28e419681e..5415503eba 100644 --- a/include/crypto/tlssession.h +++ b/include/crypto/tlssession.h @@ -208,6 +208,31 @@ typedef ssize_t (*QCryptoTLSSessionReadFunc)(void *buf, void *opaque, Error **errp); +/** + * qcrypto_tls_session_write_cork: + * @sess: the TLS session object + * + * Causes future qcrypto_tls_session_write() calls to encrypt + * and queue data, without sending on the wire. + */ +void qcrypto_tls_session_write_cork(QCryptoTLSSession *sess); + +/** + * qcrypto_tls_session_write_uncork: + * @sess: the TLS session object + * @errp: pointer to a NULL-initialized error object + * + * Attempt to send previously queued data. If the underlying + * stream is non-blocking, then only a subset of data (if any) + * may be written. To process outstanding data, this method + * must be called again until it returns 0. + * + * Returns: the number of bytes remaining to be sent, + * or -1 on error. + */ +ssize_t qcrypto_tls_session_write_uncork(QCryptoTLSSession *sess, + Error **errp); + /** * qcrypto_tls_session_set_callbacks: * @sess: the TLS session object diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h index 7e9023570d..9e9b00c034 100644 --- a/include/io/channel-tls.h +++ b/include/io/channel-tls.h @@ -50,6 +50,7 @@ struct QIOChannelTLS { QIOChannelShutdown shutdown; guint hs_ioc_tag; guint bye_ioc_tag; + size_t corked; }; /** diff --git a/io/channel-tls.c b/io/channel-tls.c index 31ec4d236d..05317ed5a3 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -21,6 +21,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qemu/module.h" +#include "qemu/iov.h" #include "io/channel-tls.h" #include "trace.h" #include "qemu/atomic.h" @@ -448,12 +449,24 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc, QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); size_t i; ssize_t done = 0; + ssize_t remain; + g_autofree struct iovec *tmpiov = NULL; + size_t ntmpiov = 0; - for (i = 0 ; i < niov ; i++) { - ssize_t ret = qcrypto_tls_session_write(tioc->session, - iov[i].iov_base, - iov[i].iov_len, - errp); + /* + * The previous write encrypted all the data, but some + * was not able to be sent on the wire when uncorked, + * so we returned a short write. The encrypted data + * will still be cached by GNUTLS and the session will + * be in a corked state. + * + * This write call will be trying to write the remaining + * plain text data again, but we must avoid sending that + * into GNUTLS. Instead flush the previously encrypted + * pending data. + */ + while (tioc->corked) { + ssize_t ret = qcrypto_tls_session_write_uncork(tioc->session, errp); if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) { if (done) { return done; @@ -463,12 +476,74 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc, } else if (ret < 0) { return -1; } + done += (tioc->corked - ret); + tioc->corked = ret; + } + + /* + * If we flushed pending data, we must discard an + * equivalent amount of plain text data from this + * write call, and then process what's left over, + * if any. + */ + if (done) { + ssize_t total = iov_size(iov, niov); + if (done == total) { + return done; + } + + tmpiov = g_new0(struct iovec, niov); + ntmpiov = iov_copy(tmpiov, niov, iov, niov, + done, total - done); + + iov = tmpiov; + niov = ntmpiov; + } + + /* + * At this point we should ony be processing "new" + * data, not seen by a previous write call so we + * send to GNUTLS as normal. + */ + qcrypto_tls_session_write_cork(tioc->session); + for (i = 0 ; i < niov ; i++) { + ssize_t ret = qcrypto_tls_session_write(tioc->session, + iov[i].iov_base, + iov[i].iov_len, + errp); + if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) { + error_setg(errp, "Unexpected TLS blocking I/O while corked"); + return -1; + } else if (ret < 0) { + return -1; + } done += ret; if (ret < iov[i].iov_len) { break; } } - return done; + + /* + * On non-blocking sockets, uncorking may not succeed + * in sending all encrypted data, so we have to check + * what was actually sent to see if GNUTLS remains in + * the corked state with pending data. + */ + remain = qcrypto_tls_session_write_uncork(tioc->session, errp); + if (remain < 0) { + return -1; + } else if (remain) { + tioc->corked = remain; + } + /* + * done == what we sent to GNUTLS for encryption & sending + * remain == subset of 'done' that was encrypted but not sent + */ + if (done && (remain == done)) { + return QIO_CHANNEL_ERR_BLOCK; + } else { + return done - remain; + } } static int qio_channel_tls_set_blocking(QIOChannel *ioc, -- 2.55.0
