| Message ID | 20220608181808.79364-1-leobras@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] QIOChannelSocket: Fix zero-copy send so socket flush works | expand |
On Wed, Jun 08, 2022 at 03:18:09PM -0300, Leonardo Bras wrote: > Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial > part of the flushing mechanism got missing: incrementing zero_copy_queued. > > Without that, the flushing interface becomes a no-op, and there is no > garantee the buffer is really sent. > > This can go as bad as causing a corruption in RAM during migration. > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX") > Reported-by: 徐闯 <xuchuangxclwt@bytedance.com> > Signed-off-by: Leonardo Bras <leobras@redhat.com> > --- > io/channel-socket.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/io/channel-socket.c b/io/channel-socket.c > index dc9c165de1..ca4cae930f 100644 > --- a/io/channel-socket.c > +++ b/io/channel-socket.c > @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > size_t fdsize = sizeof(int) * nfds; > struct cmsghdr *cmsg; > int sflags = 0; > + bool zero_copy_enabled = false; > > memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)); > > @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > #ifdef QEMU_MSG_ZEROCOPY > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > sflags = MSG_ZEROCOPY; > + zero_copy_enabled = true; > } > #endif > > @@ -592,21 +594,24 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > return QIO_CHANNEL_ERR_BLOCK; > case EINTR: > goto retry; > -#ifdef QEMU_MSG_ZEROCOPY Removing this ifdef appears incidental to the change. If this is redundant just remove it in its own patch. > case ENOBUFS: > - if (sflags & MSG_ZEROCOPY) { > + if (zero_copy_enabled) { > error_setg_errno(errp, errno, > "Process can't lock enough memory for using MSG_ZEROCOPY"); > return -1; > } > break; > -#endif > } > > error_setg_errno(errp, errno, > "Unable to write to socket"); > return -1; > } > + > + if (zero_copy_enabled) { What's wrong with if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { sioc->zero_copy_queued++; } Introducing another local variable doesn't really add value IMHO. With regards, Daniel
On Wed, Jun 08, 2022 at 07:46:43PM +0100, Daniel P. Berrangé wrote: > On Wed, Jun 08, 2022 at 03:18:09PM -0300, Leonardo Bras wrote: > > Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial > > part of the flushing mechanism got missing: incrementing zero_copy_queued. > > > > Without that, the flushing interface becomes a no-op, and there is no > > garantee the buffer is really sent. > > > > This can go as bad as causing a corruption in RAM during migration. > > > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX") > > Reported-by: 徐闯 <xuchuangxclwt@bytedance.com> > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > --- > > io/channel-socket.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/io/channel-socket.c b/io/channel-socket.c > > index dc9c165de1..ca4cae930f 100644 > > --- a/io/channel-socket.c > > +++ b/io/channel-socket.c > > @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > > size_t fdsize = sizeof(int) * nfds; > > struct cmsghdr *cmsg; > > int sflags = 0; > > + bool zero_copy_enabled = false; > > > > memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)); > > > > @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > > #ifdef QEMU_MSG_ZEROCOPY > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > > sflags = MSG_ZEROCOPY; > > + zero_copy_enabled = true; > > } > > #endif > > > > @@ -592,21 +594,24 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > > return QIO_CHANNEL_ERR_BLOCK; > > case EINTR: > > goto retry; > > -#ifdef QEMU_MSG_ZEROCOPY > > Removing this ifdef appears incidental to the change. If this is > redundant just remove it in its own patch. > > > case ENOBUFS: > > - if (sflags & MSG_ZEROCOPY) { > > + if (zero_copy_enabled) { > > error_setg_errno(errp, errno, > > "Process can't lock enough memory for using MSG_ZEROCOPY"); > > return -1; > > } > > break; > > -#endif > > } > > > > error_setg_errno(errp, errno, > > "Unable to write to socket"); > > return -1; > > } > > + > > + if (zero_copy_enabled) { > > What's wrong with > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > sioc->zero_copy_queued++; > } > > > Introducing another local variable doesn't really add value IMHO. One benefit of having that variable is we setup zero_copy_enabled once in the #ifdef and the rest code can avoid wrapping with the macro. From that pov the patch looks okay to me. Thanks,
On Wed, Jun 08, 2022 at 03:18:09PM -0300, Leonardo Bras wrote: > Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial > part of the flushing mechanism got missing: incrementing zero_copy_queued. > > Without that, the flushing interface becomes a no-op, and there is no > garantee the buffer is really sent. > > This can go as bad as causing a corruption in RAM during migration. > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX") > Reported-by: 徐闯 <xuchuangxclwt@bytedance.com> > Signed-off-by: Leonardo Bras <leobras@redhat.com> Copy Dave/Juan; Leo please remember to do so in the next posts, or no one will be picking this up. :)
Hello Daniel, On Wed, Jun 8, 2022 at 3:46 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Jun 08, 2022 at 03:18:09PM -0300, Leonardo Bras wrote: > > Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial > > part of the flushing mechanism got missing: incrementing zero_copy_queued. > > > > Without that, the flushing interface becomes a no-op, and there is no > > garantee the buffer is really sent. > > > > This can go as bad as causing a corruption in RAM during migration. > > > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX") > > Reported-by: 徐闯 <xuchuangxclwt@bytedance.com> > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > --- > > io/channel-socket.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/io/channel-socket.c b/io/channel-socket.c > > index dc9c165de1..ca4cae930f 100644 > > --- a/io/channel-socket.c > > +++ b/io/channel-socket.c > > @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > > size_t fdsize = sizeof(int) * nfds; > > struct cmsghdr *cmsg; > > int sflags = 0; > > + bool zero_copy_enabled = false; > > > > memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)); > > > > @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > > #ifdef QEMU_MSG_ZEROCOPY > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > > sflags = MSG_ZEROCOPY; > > + zero_copy_enabled = true; > > } > > #endif > > > > @@ -592,21 +594,24 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, > > return QIO_CHANNEL_ERR_BLOCK; > > case EINTR: > > goto retry; > > -#ifdef QEMU_MSG_ZEROCOPY > > Removing this ifdef appears incidental to the change. If this is > redundant just remove it in its own patch. The idea is to reduce the amount of #ifdefs as Peter suggested, because adding another ifdef here would introduce extra noise. But sure, I see no problem adding this change as a previous patch. > > > case ENOBUFS: > > - if (sflags & MSG_ZEROCOPY) { > > + if (zero_copy_enabled) { > > error_setg_errno(errp, errno, > > "Process can't lock enough memory for using MSG_ZEROCOPY"); > > return -1; > > } > > break; > > -#endif > > } > > > > error_setg_errno(errp, errno, > > "Unable to write to socket"); > > return -1; > > } > > + > > + if (zero_copy_enabled) { > > What's wrong with > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > sioc->zero_copy_queued++; > } There is nothing wrong with it, but using zero_copy_enabled as presented here will compile-out this 'if()' block if the user does not support MSG_ZEROCOPY. Best regards, Leo > > > Introducing another local variable doesn't really add value IMHO. > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Wed, Jun 08, 2022 at 04:26:10PM -0400, Peter Xu wrote: > On Wed, Jun 08, 2022 at 03:18:09PM -0300, Leonardo Bras wrote: > > Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial > > part of the flushing mechanism got missing: incrementing zero_copy_queued. > > > > Without that, the flushing interface becomes a no-op, and there is no > > garantee the buffer is really sent. > > > > This can go as bad as causing a corruption in RAM during migration. > > > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX") > > Reported-by: 徐闯 <xuchuangxclwt@bytedance.com> > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > Copy Dave/Juan; Leo please remember to do so in the next posts, or no one > will be picking this up. :) My fault, it's an io channel patch. But still good to copy relevant developers..
On Wed, Jun 8, 2022 at 5:55 PM Peter Xu <peterx@redhat.com> wrote: > > On Wed, Jun 08, 2022 at 04:26:10PM -0400, Peter Xu wrote: > > On Wed, Jun 08, 2022 at 03:18:09PM -0300, Leonardo Bras wrote: > > > Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial > > > part of the flushing mechanism got missing: incrementing zero_copy_queued. > > > > > > Without that, the flushing interface becomes a no-op, and there is no > > > garantee the buffer is really sent. > > > > > > This can go as bad as causing a corruption in RAM during migration. > > > > > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX") > > > Reported-by: 徐闯 <xuchuangxclwt@bytedance.com> > > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > > > Copy Dave/Juan; Leo please remember to do so in the next posts, or no one > > will be picking this up. :) > Thanks for letting me know. > My fault, it's an io channel patch. But still good to copy relevant > developers.. Np. Sure, I will keep in mind to add them in the next version. Oh, BTW: I will be sending a v2 shortly. > > -- > Peter Xu >
diff --git a/io/channel-socket.c b/io/channel-socket.c index dc9c165de1..ca4cae930f 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, size_t fdsize = sizeof(int) * nfds; struct cmsghdr *cmsg; int sflags = 0; + bool zero_copy_enabled = false; memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)); @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, #ifdef QEMU_MSG_ZEROCOPY if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { sflags = MSG_ZEROCOPY; + zero_copy_enabled = true; } #endif @@ -592,21 +594,24 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, return QIO_CHANNEL_ERR_BLOCK; case EINTR: goto retry; -#ifdef QEMU_MSG_ZEROCOPY case ENOBUFS: - if (sflags & MSG_ZEROCOPY) { + if (zero_copy_enabled) { error_setg_errno(errp, errno, "Process can't lock enough memory for using MSG_ZEROCOPY"); return -1; } break; -#endif } error_setg_errno(errp, errno, "Unable to write to socket"); return -1; } + + if (zero_copy_enabled) { + sioc->zero_copy_queued++; + } + return ret; } #else /* WIN32 */
Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial part of the flushing mechanism got missing: incrementing zero_copy_queued. Without that, the flushing interface becomes a no-op, and there is no garantee the buffer is really sent. This can go as bad as causing a corruption in RAM during migration. Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX") Reported-by: 徐闯 <xuchuangxclwt@bytedance.com> Signed-off-by: Leonardo Bras <leobras@redhat.com> --- io/channel-socket.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)