* [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability @ 2022-06-08 21:04 Leonardo Bras 2022-06-08 21:04 ` [PATCH v2 2/2] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras 2022-06-09 8:09 ` [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability Daniel P. Berrangé 0 siblings, 2 replies; 8+ messages in thread From: Leonardo Bras @ 2022-06-08 21:04 UTC (permalink / raw) To: Daniel P. Berrangé, Peter Xu, 徐闯, David Gilbert, Juan Quintela Cc: Leonardo Bras, qemu-devel During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were introduced, particularly at qio_channel_socket_writev(). Rewrite some of those changes so it's easier to read. ... Signed-off-by: Leonardo Bras <leobras@redhat.com> --- io/channel-socket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/io/channel-socket.c b/io/channel-socket.c index dc9c165de1..ef7c7cfbac 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,15 +594,13 @@ 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, -- 2.36.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] QIOChannelSocket: Fix zero-copy send so socket flush works 2022-06-08 21:04 [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability Leonardo Bras @ 2022-06-08 21:04 ` Leonardo Bras 2022-06-09 8:09 ` [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability Daniel P. Berrangé 1 sibling, 0 replies; 8+ messages in thread From: Leonardo Bras @ 2022-06-08 21:04 UTC (permalink / raw) To: Daniel P. Berrangé, Peter Xu, 徐闯, David Gilbert, Juan Quintela Cc: Leonardo Bras, qemu-devel 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 guarantee 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 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/io/channel-socket.c b/io/channel-socket.c index ef7c7cfbac..ca4cae930f 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -607,6 +607,11 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, "Unable to write to socket"); return -1; } + + if (zero_copy_enabled) { + sioc->zero_copy_queued++; + } + return ret; } #else /* WIN32 */ -- 2.36.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability 2022-06-08 21:04 [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability Leonardo Bras 2022-06-08 21:04 ` [PATCH v2 2/2] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras @ 2022-06-09 8:09 ` Daniel P. Berrangé 2022-06-10 1:30 ` Leonardo Bras Soares Passos 1 sibling, 1 reply; 8+ messages in thread From: Daniel P. Berrangé @ 2022-06-09 8:09 UTC (permalink / raw) To: Leonardo Bras Cc: Peter Xu, 徐闯, David Gilbert, Juan Quintela, qemu-devel On Wed, Jun 08, 2022 at 06:04:02PM -0300, Leonardo Bras wrote: > During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were > introduced, particularly at qio_channel_socket_writev(). > > Rewrite some of those changes so it's easier to read. > ... > Signed-off-by: Leonardo Bras <leobras@redhat.com> > --- > io/channel-socket.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/io/channel-socket.c b/io/channel-socket.c > index dc9c165de1..ef7c7cfbac 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; > } There should be a #else error_setg(errp, "Zero copy not supported on this platform"); return -1; #endif > #endif > > @@ -592,15 +594,13 @@ 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) { if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) avoids the #ifdef without needing to add yet another variable expressing what's already expressed in both 'flags' and 'sflags'. > error_setg_errno(errp, errno, > "Process can't lock enough memory for using MSG_ZEROCOPY"); > return -1; > } > break; > -#endif > } > > error_setg_errno(errp, errno, > -- > 2.36.1 > 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 :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability 2022-06-09 8:09 ` [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability Daniel P. Berrangé @ 2022-06-10 1:30 ` Leonardo Bras Soares Passos 2022-06-10 8:25 ` Daniel P. Berrangé 0 siblings, 1 reply; 8+ messages in thread From: Leonardo Bras Soares Passos @ 2022-06-10 1:30 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Peter Xu, 徐闯, David Gilbert, Juan Quintela, qemu-devel Hello Daniel, On Thu, Jun 9, 2022 at 5:10 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Jun 08, 2022 at 06:04:02PM -0300, Leonardo Bras wrote: > > During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were > > introduced, particularly at qio_channel_socket_writev(). > > > > Rewrite some of those changes so it's easier to read. > > ... > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > --- > > io/channel-socket.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/io/channel-socket.c b/io/channel-socket.c > > index dc9c165de1..ef7c7cfbac 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; > > } > > There should be a > > #else > error_setg(errp, "Zero copy not supported on this platform"); > return -1; > #endif > IIUC, if done as suggested, it will break every non-zero-copy call of qio_channel_socket_writev(); I think you are suggesting something like : if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { #ifdef QEMU_MSG_ZEROCOPY sflags = MSG_ZEROCOPY; zero_copy_enabled = true; // I know you suggested this out, just for example purposes #else error_setg(errp, "Zero copy not supported on this platform"); return -1; #endif } Which is supposed to fail if QIO_CHANNEL_WRITE_FLAG_ZERO_COPY is specified, but qemu does not support it at compile time. If I get the part above correctly, it would not be necessary, as qio_channel_socket_writev() is called only by qio_channel_writev_full(), which tests: if ((flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) && !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) { error_setg_errno(errp, EINVAL, "Requested Zero Copy feature is not available"); return -1; } and QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY is only set in qio_channel_socket_connect_sync(), and is conditional to QEMU_MSG_ZEROCOPY being enabled during compile time. Meaning it's the same test as before mentioned, but failing earlier. > > #endif > > > > @@ -592,15 +594,13 @@ 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) { > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) > > avoids the #ifdef without needing to add yet another > variable expressing what's already expressed in both > 'flags' and 'sflags'. Yes, it does, but at the cost of not compiling-out the zero-copy part when it's not supported, since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up meaning there will be at least one extra test for every time this function is called (the one in the next patch). An option would be testing sflags & MSG_ZEROCOPY, which would compile-out zero-copy code if it's not supported, but there was a bug in some distros where MSG_ZEROCOPY is not defined, causing the build to fail. I understand the idea of reusing those variables instead of creating a new one, but this boolean variable will most certainly be compiled-out in this function, and will allow compiling out the zero-copy code where it's not supported. Best regards, Leo > > > error_setg_errno(errp, errno, > > "Process can't lock enough memory for using MSG_ZEROCOPY"); > > return -1; > > } > > break; > > -#endif > > } > > > > error_setg_errno(errp, errno, > > -- > > 2.36.1 > > > > 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 :| > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability 2022-06-10 1:30 ` Leonardo Bras Soares Passos @ 2022-06-10 8:25 ` Daniel P. Berrangé 2022-06-13 21:21 ` Leonardo Bras Soares Passos 0 siblings, 1 reply; 8+ messages in thread From: Daniel P. Berrangé @ 2022-06-10 8:25 UTC (permalink / raw) To: Leonardo Bras Soares Passos Cc: Peter Xu, 徐闯, David Gilbert, Juan Quintela, qemu-devel On Thu, Jun 09, 2022 at 10:30:19PM -0300, Leonardo Bras Soares Passos wrote: > Hello Daniel, > > On Thu, Jun 9, 2022 at 5:10 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Wed, Jun 08, 2022 at 06:04:02PM -0300, Leonardo Bras wrote: > > > During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were > > > introduced, particularly at qio_channel_socket_writev(). > > > > > > Rewrite some of those changes so it's easier to read. > > > ... > > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > > --- > > > io/channel-socket.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/io/channel-socket.c b/io/channel-socket.c > > > index dc9c165de1..ef7c7cfbac 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; > > > } > > > > There should be a > > > > #else > > error_setg(errp, "Zero copy not supported on this platform"); > > return -1; > > #endif > > > > IIUC, if done as suggested, it will break every non-zero-copy call of > qio_channel_socket_writev(); > > I think you are suggesting something like : > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > #ifdef QEMU_MSG_ZEROCOPY > sflags = MSG_ZEROCOPY; > zero_copy_enabled = true; // I know you suggested this out, > just for example purposes > #else > error_setg(errp, "Zero copy not supported on this platform"); > return -1; > #endif > } Yes, that is what I mean. > > Which is supposed to fail if QIO_CHANNEL_WRITE_FLAG_ZERO_COPY is specified, but > qemu does not support it at compile time. Correct, the caller should have checked the ZERO_COPY feeature when they first opened the channel, and if they none the less pass ZERO_COPY when it isn't supported that is a programmer error that needs reporting. > If I get the part above correctly, it would not be necessary, as > qio_channel_socket_writev() is > called only by qio_channel_writev_full(), which tests: > > if ((flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) && > !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) { > error_setg_errno(errp, EINVAL, > "Requested Zero Copy feature is not available"); > return -1; > } Ok, so if it is checked earlier then we merely need an assert. if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { #ifdef QEMU_MSG_ZEROCOPY sflags = MSG_ZEROCOPY; zero_copy_enabled = true; #else g_assert_unreachable(); #endif > } > > > @@ -592,15 +594,13 @@ 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) { > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) > > > > avoids the #ifdef without needing to add yet another > > variable expressing what's already expressed in both > > 'flags' and 'sflags'. > > Yes, it does, but at the cost of not compiling-out the zero-copy part > when it's not supported, > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up > meaning there will be at least one extra test for every time this > function is called (the one in the next patch). The cost of a simple bit test is between negligible-and-non-existant with branch prediction. I doubt it would be possible to even measure it. 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 :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability 2022-06-10 8:25 ` Daniel P. Berrangé @ 2022-06-13 21:21 ` Leonardo Bras Soares Passos 2022-06-14 8:36 ` Daniel P. Berrangé 0 siblings, 1 reply; 8+ messages in thread From: Leonardo Bras Soares Passos @ 2022-06-13 21:21 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Peter Xu, 徐闯, David Gilbert, Juan Quintela, qemu-devel On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > [...] > Ok, so if it is checked earlier then we merely need an assert. > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > #ifdef QEMU_MSG_ZEROCOPY > sflags = MSG_ZEROCOPY; > zero_copy_enabled = true; > #else > g_assert_unreachable(); > #endif > > } Ok, I will add that in the next version. > > > > > > > @@ -592,15 +594,13 @@ 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) { > > > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) > > > > > > avoids the #ifdef without needing to add yet another > > > variable expressing what's already expressed in both > > > 'flags' and 'sflags'. > > > > Yes, it does, but at the cost of not compiling-out the zero-copy part > > when it's not supported, > > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up > > meaning there will be at least one extra test for every time this > > function is called (the one in the next patch). > > The cost of a simple bit test is between negligible-and-non-existant > with branch prediction. I doubt it would be possible to even measure > it. Yeah, you are probably right on that. So the main learning point here is that it's not worth creating a new boolean for compiling-out code that should not impact performance ? I mean, if performance-wise they should be the same, then a new variable would be just a bother for the programmer. Best regards, Leo > > 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 :| > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability 2022-06-13 21:21 ` Leonardo Bras Soares Passos @ 2022-06-14 8:36 ` Daniel P. Berrangé 2022-06-14 16:22 ` Leonardo Bras Soares Passos 0 siblings, 1 reply; 8+ messages in thread From: Daniel P. Berrangé @ 2022-06-14 8:36 UTC (permalink / raw) To: Leonardo Bras Soares Passos Cc: Peter Xu, 徐闯, David Gilbert, Juan Quintela, qemu-devel On Mon, Jun 13, 2022 at 06:21:18PM -0300, Leonardo Bras Soares Passos wrote: > On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > [...] > > > Ok, so if it is checked earlier then we merely need an assert. > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > > #ifdef QEMU_MSG_ZEROCOPY > > sflags = MSG_ZEROCOPY; > > zero_copy_enabled = true; > > #else > > g_assert_unreachable(); > > #endif > > > } > > Ok, I will add that in the next version. > > > > > > > > > > > > @@ -592,15 +594,13 @@ 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) { > > > > > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) > > > > > > > > avoids the #ifdef without needing to add yet another > > > > variable expressing what's already expressed in both > > > > 'flags' and 'sflags'. > > > > > > Yes, it does, but at the cost of not compiling-out the zero-copy part > > > when it's not supported, > > > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up > > > meaning there will be at least one extra test for every time this > > > function is called (the one in the next patch). > > > > The cost of a simple bit test is between negligible-and-non-existant > > with branch prediction. I doubt it would be possible to even measure > > it. > > Yeah, you are probably right on that. > So the main learning point here is that it's not worth creating a new > boolean for compiling-out > code that should not impact performance ? As ever "it depends" so there's no hard rule, and sometimes it can verge on bikeshed colouring :-) I didn't like the variable in this case, because it introduces a 3rd variable to the method for representing whether zero copy is need, which is excessive. I'm not a fan of redundancy as it can often then lead to inconsistency. So it would need a compelling reason why it is better, which is difficult for such a simple method. If the code was more complex, a variable might have benefit of clarity, but in this case IMHO it was just overkill. 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 :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability 2022-06-14 8:36 ` Daniel P. Berrangé @ 2022-06-14 16:22 ` Leonardo Bras Soares Passos 0 siblings, 0 replies; 8+ messages in thread From: Leonardo Bras Soares Passos @ 2022-06-14 16:22 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Peter Xu, 徐闯, David Gilbert, Juan Quintela, qemu-devel On Tue, Jun 14, 2022 at 5:36 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Mon, Jun 13, 2022 at 06:21:18PM -0300, Leonardo Bras Soares Passos wrote: > > On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > > [...] > > > > > Ok, so if it is checked earlier then we merely need an assert. > > > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > > > #ifdef QEMU_MSG_ZEROCOPY > > > sflags = MSG_ZEROCOPY; > > > zero_copy_enabled = true; > > > #else > > > g_assert_unreachable(); > > > #endif > > > > } > > > > Ok, I will add that in the next version. > > > > > > > > > > > > > > > > > @@ -592,15 +594,13 @@ 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) { > > > > > > > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) > > > > > > > > > > avoids the #ifdef without needing to add yet another > > > > > variable expressing what's already expressed in both > > > > > 'flags' and 'sflags'. > > > > > > > > Yes, it does, but at the cost of not compiling-out the zero-copy part > > > > when it's not supported, > > > > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up > > > > meaning there will be at least one extra test for every time this > > > > function is called (the one in the next patch). > > > > > > The cost of a simple bit test is between negligible-and-non-existant > > > with branch prediction. I doubt it would be possible to even measure > > > it. > > > > Yeah, you are probably right on that. > > So the main learning point here is that it's not worth creating a new > > boolean for compiling-out > > code that should not impact performance ? > > As ever "it depends" so there's no hard rule, and sometimes it can > verge on bikeshed colouring :-) > > I didn't like the variable in this case, because it introduces a 3rd > variable to the method for representing whether zero copy is need, > which is excessive. I'm not a fan of redundancy as it can often then > lead to inconsistency. So it would need a compelling reason why it is > better, which is difficult for such a simple method. If the code was > more complex, a variable might have benefit of clarity, but in this > case IMHO it was just overkill. I see. Thanks for the clarification! Best regards, Leo > > 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 :| > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-14 16:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-08 21:04 [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability Leonardo Bras 2022-06-08 21:04 ` [PATCH v2 2/2] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras 2022-06-09 8:09 ` [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability Daniel P. Berrangé 2022-06-10 1:30 ` Leonardo Bras Soares Passos 2022-06-10 8:25 ` Daniel P. Berrangé 2022-06-13 21:21 ` Leonardo Bras Soares Passos 2022-06-14 8:36 ` Daniel P. Berrangé 2022-06-14 16:22 ` Leonardo Bras Soares Passos
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.