| Message ID | 20220428144052.263382-7-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/11] tests: fix encoding of IP addresses in x509 certs | expand |
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
>
>
Hello Dave, On Thu, Apr 28, 2022 at 1:20 PM Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > 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 About MSG_ZEROCOPY error: I tracked down how the test happened, downloaded the same docker image from the tests(opensuse-leap-15.2), and took a look at the filesystem for the MSG_ZEROCOPY define, which I could not find anywhere. Then I took a look into /usr/include/bits/socket.h, which is where RHEL has MSG_ZEROCOPY defined. Zypper defines it as been provided by glibc-devel, which is versioned at 2.26-lp152.26.12.1. I then took a look at https://sourceware.org/git/glibc.git, and found commit 78cde19f62 that introduces MSG_ZEROCOPY. The first version that has this commit is glibc-2.27. So, basically, this means opensuse-leap-15.2 glibc version does not support MSG_ZEROCOPY. Based on that, I had a few ideas on how to solve the CI bug: 1 - Propose a backport of this patch (few comments + single define) for leap-15.x, wait for them to accept and update the version in qemu CI. (TBH I have no idea how the opensuse community works, I just suppose it could be a way of tackling this.) 2 - include an #ifndef MSG_ZEROCOPY #define MSG_ZEROCOPY 0x4000000 #endif in code, which is ugly IMHO, but will be fast and clean. 3 - In CI, patch /usr/include/bits/socket.h before building, which will also work fine, but defeats the purpose of keeping qemu building on the platform. Among the above, I would go with (2), as it seems a reasonable way of dealing with this. Does anyone else have any further suggestions, or know how this kind of issue is generally solved in qemu? Best regards, Leo > > > 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 > >
Leo, On Fri, Apr 29, 2022 at 11:40:44PM -0300, Leonardo Bras Soares Passos wrote: > Does anyone else have any further suggestions, or know how this kind of issue > is generally solved in qemu? I've no solid idea why it can't see MSG_ZEROCOPY defined in the specific environment, but when I was looking at bits/socket.h I saw this: #ifndef _SYS_SOCKET_H # error "Never include <bits/socket.h> directly; use <sys/socket.h> instead." #endif Maybe worth a shot to do a replacement in all cases?
Hello Peter, On Mon, May 2, 2022 at 8:52 PM Peter Xu <peterx@redhat.com> wrote: > > Leo, > > On Fri, Apr 29, 2022 at 11:40:44PM -0300, Leonardo Bras Soares Passos wrote: > > Does anyone else have any further suggestions, or know how this kind of issue > > is generally solved in qemu? > > I've no solid idea why it can't see MSG_ZEROCOPY defined in the specific > environment, but when I was looking at bits/socket.h I saw this: > > #ifndef _SYS_SOCKET_H > # error "Never include <bits/socket.h> directly; use <sys/socket.h> instead." > #endif > > Maybe worth a shot to do a replacement in all cases? > Sure, no problem with this, I will update for v11. (Or should I send a different patch since Dave has already merged in his tree?) But it should not interfere in MSG_ZEROCOPY definition: > > I tracked down how the test happened, downloaded the same docker image from the > > tests(opensuse-leap-15.2), and took a look at the filesystem for the > > MSG_ZEROCOPY define, which I could not find anywhere. By this, I mean I did a 'grep MSG_ZEROCOPY -r /' and could not find anything, so it's probably not defined anywhere in the fs. > -- > Peter Xu > Thanks Peter! Best regards, Leo
On Mon, May 02, 2022 at 09:12:53PM -0300, Leonardo Bras Soares Passos wrote: > Hello Peter, > > On Mon, May 2, 2022 at 8:52 PM Peter Xu <peterx@redhat.com> wrote: > > > > Leo, > > > > On Fri, Apr 29, 2022 at 11:40:44PM -0300, Leonardo Bras Soares Passos wrote: > > > Does anyone else have any further suggestions, or know how this kind of issue > > > is generally solved in qemu? > > > > I've no solid idea why it can't see MSG_ZEROCOPY defined in the specific > > environment, but when I was looking at bits/socket.h I saw this: > > > > #ifndef _SYS_SOCKET_H > > # error "Never include <bits/socket.h> directly; use <sys/socket.h> instead." > > #endif > > > > Maybe worth a shot to do a replacement in all cases? > > > > Sure, no problem with this, I will update for v11. > (Or should I send a different patch since Dave has already merged in his tree?) > > But it should not interfere in MSG_ZEROCOPY definition: > > > > I tracked down how the test happened, downloaded the same docker image from the > > > tests(opensuse-leap-15.2), and took a look at the filesystem for the > > > MSG_ZEROCOPY define, which I could not find anywhere. > > By this, I mean I did a 'grep MSG_ZEROCOPY -r /' and could not find anything, so > it's probably not defined anywhere in the fs. What you described gives me the feeling that the distro seems to have had mismatched versions of asm-generic/socket.h (who should define SO_ZEROCOPY), and bits/socket.h (who should define MSG_ZEROCOPY). Let's first replace it with sys/socket.h, then one trick you could consider play with (even if any env could have broken headers) that I thought of, is you can put your code into: #if defined(MSG_ZEROCOPY) && defined(SO_ZEROCOPY) ... #endif Blocks. Just to avoid assuming CONFIG_LINUX will be the same.
* Leonardo Bras Soares Passos (lsoaresp@redhat.com) wrote: > Hello Dave, > > On Thu, Apr 28, 2022 at 1:20 PM Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > > > 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 > > About MSG_ZEROCOPY error: > > I tracked down how the test happened, downloaded the same docker image from the > tests(opensuse-leap-15.2), and took a look at the filesystem for the > MSG_ZEROCOPY define, which I could not find anywhere. > > Then I took a look into /usr/include/bits/socket.h, which is where RHEL has > MSG_ZEROCOPY defined. Zypper defines it as been provided by glibc-devel, which > is versioned at 2.26-lp152.26.12.1. > > I then took a look at https://sourceware.org/git/glibc.git, and found commit > 78cde19f62 that introduces MSG_ZEROCOPY. The first version that has this commit > is glibc-2.27. > > So, basically, this means opensuse-leap-15.2 glibc version does not support > MSG_ZEROCOPY. Based on that, I had a few ideas on how to solve the CI bug: > 1 - Propose a backport of this patch (few comments + single define) for > leap-15.x, wait for them to accept and update the version in qemu CI. > (TBH I have no idea how the opensuse community works, I just suppose it could > be a way of tackling this.) > 2 - include an #ifndef MSG_ZEROCOPY #define MSG_ZEROCOPY 0x4000000 #endif in > code, which is ugly IMHO, but will be fast and clean. > 3 - In CI, patch /usr/include/bits/socket.h before building, which will also > work fine, but defeats the purpose of keeping qemu building on the platform. > > Among the above, I would go with (2), as it seems a reasonable way of dealing > with this. Right we need to run on the current set of distros, so we need to do (2); it's not clear if we need to make trying to enable the feature fail if the host doesn't support it. Now, having said that, you might also want to file an Opensuse bug suggesting they do (1). Dave > Does anyone else have any further suggestions, or know how this kind of issue > is generally solved in qemu? > > Best regards, > Leo > > > > > > > > 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 > > > > >
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 = {