Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[Leonardo Bras Soares Passos]

On Thu, May 5, 2022 at 12:55 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, May 05, 2022 at 12:42:47PM -0300, Leonardo Bras Soares Passos wrote:
> >
> > Hello Daniel,
> >
> > But what if this gets compiled in a Linux system without MSG_ZEROCOPY 
> > support?
> > As qapi will have zero-copy-send as an option we could have this scenario:
> >
> > - User request migration using zero-copy-send
> > - multifd_save_setup() will set write_flags = 
> > QIO_CHANNEL_WRITE_FLAG_ZERO_COPY
> > - In qio_channel_socket_connect_sync(): setsockopt() part will be
> > compiled-out, so no error here
> > - above part in qio_channel_socket_writev() will be commented-out,
> > which means write_flags will be ignored
> > - sflags will not contain MSG_ZEROCOPY, so sendmsg() will use copy-mode
> > - migration will succeed
> >
> > In the above case, the user has all the reason to think migration is
> > using MSG_ZEROCOPY, but in fact it's quietly falling back to
> > copy-mode.
>
> I think we're ok because qio_channel_writev_full() does
>
>     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 since there's no way for QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY to
> get set when MSG_ZEROCOPY is compiled out, we'll trigger the error
> condition.

Oh, that's right. It will fail in the first writev(), I was just
considering failing during setup.

>
> > That's why I suggested creating a 'global' config usiing SO_ZEROCOPY &
> > MSG_ZEROCOPY & CONFIG_LINUX so we can use in qapi and have no chance
> > of even offering zero-copy-send if we don't have it.
> >
> > Another local option is to do implement your suggestions, and also
> > change qio_channel_socket_connect_sync() so it returns an error if
> > MSG_ZEROCOPY && SO_ZEROCOPY is not present, such as:
> >
> > +#ifdef CONFIG_LINUX
> > +#if defined(MSG_ZEROCOPY)  && defined(SO_ZEROCOPY)
> > +    int ret, v = 1;
> > +    ret = setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &v, sizeof(v));
> > +    if (ret == 0) {
> > +        /* Zero copy available on host */
> > +        qio_channel_set_feature(QIO_CHANNEL(ioc),
> > +                                QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY);
> > +    }
> > +#else
> > +    error_setg_errno(errp, errno,"MSG_ZEROCOPY not available");
> > +    return -1;
> > +#endif
> > +#endif
>
> Do we actually need the ifdef CONFIG_LINUX bit at all ?
>
> Sufficient to just have the check for MSG_ZEROCOPY + SO_ZEROCOPY,
> which will fail on non-Linux anyway.

By some include issue, or by future implementations we can have
MSG_ZEROCOPY or SO_ZEROCOPY getting defined in OS other than Linux,
which would introduce some headaches.

Since you pointed out that migration will fail on writev, the above
piece of code is not necessary.
We could have a local define that equals to (MSG_ZEROCOPY &&
SO_ZEROCOPY && CONFIG_LINUX) so that we can make the code simpler
where needed.

I will work on a v12 and send it here.

Best regards,
Leo