[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 06/11] QIOChannelSocket: Implement io_writev zero copy flag &
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PULL 06/11] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX |
Date: |
Thu, 28 Apr 2022 17:20:53 +0100 |
User-agent: |
Mutt/2.2.1 (2022-02-19) |
Leo:
Unfortunately this is failing a couple of CI tests; the MSG_ZEROCOPY
one I guess is the simpler one; I think Stefanha managed to find the
liburing fix for the __kernel_timespec case, but that looks like a bit
more fun!
Dave
Job #2390848140 ( https://gitlab.com/dagrh/qemu/-/jobs/2390848140/raw )
Name: build-system-alpine
In file included from /usr/include/linux/errqueue.h:6,
from ../io/channel-socket.c:29:
/usr/include/linux/time_types.h:7:8: error: redefinition of 'struct
__kernel_timespec'
7 | struct __kernel_timespec {
| ^~~~~~~~~~~~~~~~~
In file included from /usr/include/liburing.h:19,
from /builds/dagrh/qemu/include/block/aio.h:18,
from /builds/dagrh/qemu/include/io/channel.h:26,
from /builds/dagrh/qemu/include/io/channel-socket.h:24,
from ../io/channel-socket.c:24:
/usr/include/liburing/compat.h:9:8: note: originally defined here
9 | struct __kernel_timespec {
| ^~~~~~~~~~~~~~~~~
----
Name: build-system-opensuse
https://gitlab.com/dagrh/qemu/-/jobs/2390848160/raw
../io/channel-socket.c: In function ‘qio_channel_socket_writev’:
../io/channel-socket.c:578:18: error: ‘MSG_ZEROCOPY’ undeclared (first use
in this function); did you mean ‘SO_ZEROCOPY’?
sflags = MSG_ZEROCOPY;
^~~~~~~~~~~~
SO_ZEROCOPY
../io/channel-socket.c:578:18: note: each undeclared identifier is reported
only once for each function it appears in
* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: Leonardo Bras <leobras@redhat.com>
>
> For CONFIG_LINUX, implement the new zero copy flag and the optional callback
> io_flush on QIOChannelSocket, but enables it only when MSG_ZEROCOPY
> feature is available in the host kernel, which is checked on
> qio_channel_socket_connect_sync()
>
> qio_channel_socket_flush() was implemented by counting how many times
> sendmsg(...,MSG_ZEROCOPY) was successfully called, and then reading the
> socket's error queue, in order to find how many of them finished sending.
> Flush will loop until those counters are the same, or until some error occurs.
>
> Notes on using writev() with QIO_CHANNEL_WRITE_FLAG_ZERO_COPY:
> 1: Buffer
> - As MSG_ZEROCOPY tells the kernel to use the same user buffer to avoid
> copying,
> some caution is necessary to avoid overwriting any buffer before it's sent.
> If something like this happen, a newer version of the buffer may be sent
> instead.
> - If this is a problem, it's recommended to call qio_channel_flush() before
> freeing
> or re-using the buffer.
>
> 2: Locked memory
> - When using MSG_ZERCOCOPY, the buffer memory will be locked after queued, and
> unlocked after it's sent.
> - Depending on the size of each buffer, and how often it's sent, it may
> require
> a larger amount of locked memory than usually available to non-root user.
> - If the required amount of locked memory is not available, writev_zero_copy
> will return an error, which can abort an operation like migration,
> - Because of this, when an user code wants to add zero copy as a feature, it
> requires a mechanism to disable it, so it can still be accessible to less
> privileged users.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Message-Id: <20220426230654.637939-3-leobras@redhat.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> include/io/channel-socket.h | 2 +
> io/channel-socket.c | 108 ++++++++++++++++++++++++++++++++++--
> 2 files changed, 106 insertions(+), 4 deletions(-)
>
> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> index e747e63514..513c428fe4 100644
> --- a/include/io/channel-socket.h
> +++ b/include/io/channel-socket.h
> @@ -47,6 +47,8 @@ struct QIOChannelSocket {
> socklen_t localAddrLen;
> struct sockaddr_storage remoteAddr;
> socklen_t remoteAddrLen;
> + ssize_t zero_copy_queued;
> + ssize_t zero_copy_sent;
> };
>
>
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 696a04dc9c..1dd85fc1ef 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -25,6 +25,10 @@
> #include "io/channel-watch.h"
> #include "trace.h"
> #include "qapi/clone-visitor.h"
> +#ifdef CONFIG_LINUX
> +#include <linux/errqueue.h>
> +#include <bits/socket.h>
> +#endif
>
> #define SOCKET_MAX_FDS 16
>
> @@ -54,6 +58,8 @@ qio_channel_socket_new(void)
>
> sioc = QIO_CHANNEL_SOCKET(object_new(TYPE_QIO_CHANNEL_SOCKET));
> sioc->fd = -1;
> + sioc->zero_copy_queued = 0;
> + sioc->zero_copy_sent = 0;
>
> ioc = QIO_CHANNEL(sioc);
> qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
> @@ -153,6 +159,16 @@ int qio_channel_socket_connect_sync(QIOChannelSocket
> *ioc,
> return -1;
> }
>
> +#ifdef CONFIG_LINUX
> + int ret, v = 1;
> + ret = setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &v, sizeof(v));
> + if (ret == 0) {
> + /* Zero copy available on host */
> + qio_channel_set_feature(QIO_CHANNEL(ioc),
> + QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY);
> + }
> +#endif
> +
> return 0;
> }
>
> @@ -533,6 +549,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)];
> size_t fdsize = sizeof(int) * nfds;
> struct cmsghdr *cmsg;
> + int sflags = 0;
>
> memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
>
> @@ -557,15 +574,27 @@ static ssize_t qio_channel_socket_writev(QIOChannel
> *ioc,
> memcpy(CMSG_DATA(cmsg), fds, fdsize);
> }
>
> + if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> + sflags = MSG_ZEROCOPY;
> + }
> +
> retry:
> - ret = sendmsg(sioc->fd, &msg, 0);
> + ret = sendmsg(sioc->fd, &msg, sflags);
> if (ret <= 0) {
> - if (errno == EAGAIN) {
> + switch (errno) {
> + case EAGAIN:
> return QIO_CHANNEL_ERR_BLOCK;
> - }
> - if (errno == EINTR) {
> + case EINTR:
> goto retry;
> + case ENOBUFS:
> + if (sflags & MSG_ZEROCOPY) {
> + error_setg_errno(errp, errno,
> + "Process can't lock enough memory for using
> MSG_ZEROCOPY");
> + return -1;
> + }
> + break;
> }
> +
> error_setg_errno(errp, errno,
> "Unable to write to socket");
> return -1;
> @@ -659,6 +688,74 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> }
> #endif /* WIN32 */
>
> +
> +#ifdef CONFIG_LINUX
> +static int qio_channel_socket_flush(QIOChannel *ioc,
> + Error **errp)
> +{
> + QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> + struct msghdr msg = {};
> + struct sock_extended_err *serr;
> + struct cmsghdr *cm;
> + char control[CMSG_SPACE(sizeof(*serr))];
> + int received;
> + int ret = 1;
> +
> + msg.msg_control = control;
> + msg.msg_controllen = sizeof(control);
> + memset(control, 0, sizeof(control));
> +
> + while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
> + received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
> + if (received < 0) {
> + switch (errno) {
> + case EAGAIN:
> + /* Nothing on errqueue, wait until something is available */
> + qio_channel_wait(ioc, G_IO_ERR);
> + continue;
> + case EINTR:
> + continue;
> + default:
> + error_setg_errno(errp, errno,
> + "Unable to read errqueue");
> + return -1;
> + }
> + }
> +
> + cm = CMSG_FIRSTHDR(&msg);
> + if (cm->cmsg_level != SOL_IP &&
> + cm->cmsg_type != IP_RECVERR) {
> + error_setg_errno(errp, EPROTOTYPE,
> + "Wrong cmsg in errqueue");
> + return -1;
> + }
> +
> + serr = (void *) CMSG_DATA(cm);
> + if (serr->ee_errno != SO_EE_ORIGIN_NONE) {
> + error_setg_errno(errp, serr->ee_errno,
> + "Error on socket");
> + return -1;
> + }
> + if (serr->ee_origin != SO_EE_ORIGIN_ZEROCOPY) {
> + error_setg_errno(errp, serr->ee_origin,
> + "Error not from zero copy");
> + return -1;
> + }
> +
> + /* No errors, count successfully finished sendmsg()*/
> + sioc->zero_copy_sent += serr->ee_data - serr->ee_info + 1;
> +
> + /* If any sendmsg() succeeded using zero copy, return 0 at the end */
> + if (serr->ee_code != SO_EE_CODE_ZEROCOPY_COPIED) {
> + ret = 0;
> + }
> + }
> +
> + return ret;
> +}
> +
> +#endif /* CONFIG_LINUX */
> +
> static int
> qio_channel_socket_set_blocking(QIOChannel *ioc,
> bool enabled,
> @@ -789,6 +886,9 @@ static void qio_channel_socket_class_init(ObjectClass
> *klass,
> ioc_klass->io_set_delay = qio_channel_socket_set_delay;
> ioc_klass->io_create_watch = qio_channel_socket_create_watch;
> ioc_klass->io_set_aio_fd_handler = qio_channel_socket_set_aio_fd_handler;
> +#ifdef CONFIG_LINUX
> + ioc_klass->io_flush = qio_channel_socket_flush;
> +#endif
> }
>
> static const TypeInfo qio_channel_socket_info = {
> --
> 2.35.1
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
- [PULL 00/11] migration queue, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 06/11] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX, Dr. David Alan Gilbert (git), 2022/04/28
- Re: [PULL 06/11] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX,
Dr. David Alan Gilbert <=
- [PULL 01/11] tests: fix encoding of IP addresses in x509 certs, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 03/11] tests: convert multifd migration tests to use common helper, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 05/11] QIOChannel: Add flags on io_writev and introduce io_flush callback, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 04/11] tests: ensure migration status isn't reported as failed, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 07/11] migration: Add zero-copy-send parameter for QMP/HMP for Linux, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 08/11] migration: Add migrate_use_tls() helper, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 11/11] multifd: Implement zero copy write in multifd migration (multifd-zero-copy), Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 09/11] multifd: multifd_send_sync_main now returns negative on error, Dr. David Alan Gilbert (git), 2022/04/28
- [PULL 02/11] tests: convert XBZRLE migration test to use common helper, Dr. David Alan Gilbert (git), 2022/04/28