Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability
On Tue, Jun 14, 2022 at 5:36 AM Daniel P. Berrangé 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é
> > 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 :|
>
Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability
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é
> 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 :|
Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability
On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé 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 :|
>
Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability
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é 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
> > > ---
> > > 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 :|
Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability
Hello Daniel,
On Thu, Jun 9, 2022 at 5:10 AM Daniel P. Berrangé 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
> > ---
> > 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 :|
>
Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability
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
> ---
> 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 :|
[PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability
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
---
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
