From patchwork Fri Jun 7 22:14:14 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 1112320 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=209.51.188.17; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 45LHRK1tHKz9s9y for ; Sat, 8 Jun 2019 08:35:23 +1000 (AEST) Received: from localhost ([::1]:53994 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hZNSD-0007FB-4b for incoming@patchwork.ozlabs.org; Fri, 07 Jun 2019 18:35:21 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:52945) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hZN7s-0002rU-0q for qemu-devel@nongnu.org; Fri, 07 Jun 2019 18:14:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hZN7q-0004JR-Dm for qemu-devel@nongnu.org; Fri, 07 Jun 2019 18:14:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36838) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hZN7q-0004GX-4p for qemu-devel@nongnu.org; Fri, 07 Jun 2019 18:14:18 -0400 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D064308339A for ; Fri, 7 Jun 2019 22:14:17 +0000 (UTC) Received: from blue.redhat.com (ovpn-116-85.phx2.redhat.com [10.3.116.85]) by smtp.corp.redhat.com (Postfix) with ESMTP id BB544600C0; Fri, 7 Jun 2019 22:14:16 +0000 (UTC) From: Eric Blake To: qemu-devel@nongnu.org Date: Fri, 7 Jun 2019 17:14:14 -0500 Message-Id: <20190607221414.15962-1-eblake@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Fri, 07 Jun 2019 22:14:17 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH] RFC: qio: Improve corking of TLS sessions X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" Our current implementation of qio_channel_set_cork() is pointless for TLS sessions: we block the underlying channel, but still hand things piecemeal to gnutls which then produces multiple encryption packets. Better is to directly use gnutls corking, which collects multiple inputs into a single encryption packet. Signed-off-by: Eric Blake --- RFC because unfortunately, I'm not sure I like the results. My test case (using latest nbdkit.git) involves sending 10G of random data over NBD using parallel writes (and doing nothing on the server end; this is all about timing the data transfer): $ dd if=/dev/urandom of=rand bs=1M count=10k $ time nbdkit -p 10810 --tls=require --tls-verify-peer \ --tls-psk=/tmp/keys.psk --filter=stats null 10g statsfile=/dev/stderr \ --run '~/qemu/qemu-img convert -f raw -W -n --target-image-opts \ --object tls-creds-psk,id=tls0,endpoint=client,dir=/tmp,username=eblake \ rand driver=nbd,server.type=inet,server.host=localhost,server.port=10810,tls-creds=tls0' Pre-patch, I measured: real 0m34.536s user 0m29.264s sys 0m4.014s while post-patch, it changed to: real 0m36.055s user 0m27.685s sys 0m10.138s Less time spent in user space, but for this particular qemu-img behavior (writing 2M chunks at a time), gnutls is now uncorking huge packets and the kernel is doing enough extra work that the overall program actually takes longer. :( For smaller I/O patterns, the effects of corking are more likely to be beneficial, but I don't have a ready test case to produce that pattern (short of creating a guest running fio on a device backed by nbd). Ideas for improvements are welcome; see my recent thread on the libguestfs about how TCP_CORK is already a painful interface (it requires additional syscalls), and that we may be better off teaching qio_channel_writev about taking a flag similar to send(,MSG_MORE), which can achieve the same effect as setsockopt(TCP_CORK) but in fewer syscalls: https://www.redhat.com/archives/libguestfs/2019-June/msg00078.html https://www.redhat.com/archives/libguestfs/2019-June/msg00081.html Another idea might be teaching channel-tls.c to be smarter about the maximum size of data it is willing to cork, as well as to autocork during writev (it's a shame that gnutls doesn't have a sendmsg counterpart for sending vectors). And after all, writev already auto-corks for sockets, which is why we already went to the effort of allowing clients to use writev-like interfaces to qio channels, whether or not we also add in an ability to exploit MSG_MORE when we have back-to-back writevs to further group where it makes sense. --- include/crypto/tlssession.h | 15 +++++++++++++++ include/io/channel.h | 4 ++-- crypto/tlssession.c | 16 ++++++++++++++++ io/channel-socket.c | 3 ++- io/channel-tls.c | 9 ++++++--- io/channel-websock.c | 3 ++- io/channel.c | 11 ++++++++++- 7 files changed, 53 insertions(+), 8 deletions(-) diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h index 1c7414e4ffdd..451f58c2c742 100644 --- a/include/crypto/tlssession.h +++ b/include/crypto/tlssession.h @@ -319,4 +319,19 @@ int qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess, */ char *qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess); +/** + * qcrypto_tls_session_cork: + * @sess: the TLS session object + * @enabled: the desired cork status + * + * Update the cork status of the session. If @enabled is true, this is + * a hint that the next few writes should be batched together until + * the session is uncorked again. If false, then proceed to write + * batched data, and it is safe to call this in a loop in case + * flushing the queue would block. + * + * Returns: 0 for success, or -EAGAIN if uncorking is incomplete. + */ +int qcrypto_tls_session_cork(QCryptoTLSSession *sess, bool enabled); + #endif /* QCRYPTO_TLSSESSION_H */ diff --git a/include/io/channel.h b/include/io/channel.h index 59460cb1ec7a..e9565b9f7d65 100644 --- a/include/io/channel.h +++ b/include/io/channel.h @@ -129,8 +129,8 @@ struct QIOChannelClass { int (*io_shutdown)(QIOChannel *ioc, QIOChannelShutdown how, Error **errp); - void (*io_set_cork)(QIOChannel *ioc, - bool enabled); + int (*io_set_cork)(QIOChannel *ioc, + bool enabled); void (*io_set_delay)(QIOChannel *ioc, bool enabled); off_t (*io_seek)(QIOChannel *ioc, diff --git a/crypto/tlssession.c b/crypto/tlssession.c index c3a920dfe80e..6ef5d9001375 100644 --- a/crypto/tlssession.c +++ b/crypto/tlssession.c @@ -547,6 +547,17 @@ qcrypto_tls_session_get_peer_name(QCryptoTLSSession *session) return NULL; } +int qcrypto_tls_session_cork(QCryptoTLSSession *session, bool enabled) +{ + return 0; + if (enabled) { + gnutls_record_cork(session->handle); + } else if (gnutls_record_uncork(session->handle, 0) < 0) { + return -EAGAIN; + } + return 0; +} + #else /* ! CONFIG_GNUTLS */ @@ -639,4 +650,9 @@ qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess) return NULL; } +int qcrypto_tls_session_cork(QCryptoTLSSession *sess, bool enabled) +{ + return 0; +} + #endif diff --git a/io/channel-socket.c b/io/channel-socket.c index bc5f80e780eb..44eb85cd2ba4 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -669,7 +669,7 @@ qio_channel_socket_set_delay(QIOChannel *ioc, } -static void +static int qio_channel_socket_set_cork(QIOChannel *ioc, bool enabled) { @@ -677,6 +677,7 @@ qio_channel_socket_set_cork(QIOChannel *ioc, int v = enabled ? 1 : 0; socket_set_cork(sioc->fd, v); + return 0; } diff --git a/io/channel-tls.c b/io/channel-tls.c index c98ead21b01e..93162d5ecc85 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -346,12 +346,15 @@ static void qio_channel_tls_set_delay(QIOChannel *ioc, qio_channel_set_delay(tioc->master, enabled); } -static void qio_channel_tls_set_cork(QIOChannel *ioc, - bool enabled) +static int qio_channel_tls_set_cork(QIOChannel *ioc, + bool enabled) { QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); - qio_channel_set_cork(tioc->master, enabled); + if (qcrypto_tls_session_cork(tioc->session, enabled) == -EAGAIN) { + return QIO_CHANNEL_ERR_BLOCK; + } + return 0; } static int qio_channel_tls_shutdown(QIOChannel *ioc, diff --git a/io/channel-websock.c b/io/channel-websock.c index 77d30f0e4aa4..a288e21a2a75 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -1186,12 +1186,13 @@ static void qio_channel_websock_set_delay(QIOChannel *ioc, qio_channel_set_delay(tioc->master, enabled); } -static void qio_channel_websock_set_cork(QIOChannel *ioc, +static int qio_channel_websock_set_cork(QIOChannel *ioc, bool enabled) { QIOChannelWebsock *tioc = QIO_CHANNEL_WEBSOCK(ioc); qio_channel_set_cork(tioc->master, enabled); + return 0; } static int qio_channel_websock_shutdown(QIOChannel *ioc, diff --git a/io/channel.c b/io/channel.c index 2a26c2a2c0b9..3510912465ac 100644 --- a/io/channel.c +++ b/io/channel.c @@ -379,7 +379,16 @@ void qio_channel_set_cork(QIOChannel *ioc, QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); if (klass->io_set_cork) { - klass->io_set_cork(ioc, enabled); + int r = klass->io_set_cork(ioc, enabled); + + while (r == QIO_CHANNEL_ERR_BLOCK) { + if (qemu_in_coroutine()) { + qio_channel_yield(ioc, G_IO_OUT); + } else { + qio_channel_wait(ioc, G_IO_OUT); + } + r = klass->io_set_cork(ioc, enabled); + } } }