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