Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Daniel P . Berrangé]

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 :|