| Message ID | 20220608210403.84006-1-leobras@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] QIOChannelSocket: Reduce ifdefs to improve readability | expand |
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
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 :| >
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
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 :| >
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
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 :| >
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,
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(-)