On Wed, Oct 13, 2021 at 3:18 AM Peter Xu <pet...@redhat.com> wrote:
>
> On Sat, Oct 09, 2021 at 04:56:12AM -0300, Leonardo Bras wrote:
> > @@ -154,6 +161,17 @@ int qio_channel_socket_connect_sync(QIOChannelSocket
> > *ioc,
> > return -1;
> > }
> >
> > +#ifdef CONFIG_LINUX
> > + ret = qemu_setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &v, sizeof(v));
> > + if (ret < 0) {
> > + /* Zerocopy not available on host */
> > + return 0;
> > + }
> > +
> > + qio_channel_set_feature(QIO_CHANNEL(ioc),
> > + QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY);
>
> This is okay I think, but looks a bit weird. Maybe nicer to be written as:
>
> #if LINUX
> ret = setsockopt();
> if (ret == 0) {
> qio_channel_set_feature(...);
> }
> #endif
> return 0;
>
> ?
Yeah, I also questioned myself about this one.
At the time I ended up writing like this because the lines above used
the behavior "if error, then exit/abort", and so I thought that this
would be the better way to include this feature.
But I did not consider that this is not an error exit, but a 'maybe
feature instead'.
So, I will change that like you suggested.
>
> > +#endif
> > +
> > return 0;
> > }
>
> [...]
>
> > +static ssize_t qio_channel_socket_writev_zerocopy(QIOChannel *ioc,
> > + const struct iovec *iov,
> > + size_t niov,
> > + Error **errp)
> > +{
> > + QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> > + ssize_t ret;
> > +
> > + ret = qio_channel_socket_writev_flags(ioc, iov, niov, NULL, 0,
> > + MSG_ZEROCOPY, errp);
> > + if (ret == QIO_CHANNEL_ERR_NOBUFS) {
> > + if (errp && *errp) {
>
> Hmm this seems wrong, *errp should be NULL in most cases, meanwhile I think
> error_setg*() takes care of errp==NULL too, so maybe we can drop this?
Yeah, you are correct.
I ended up confused about how to use err, thanks for making it more clear!
>
> > + error_setg_errno(errp, errno,
> > + "Process can't lock enough memory for using
> > MSG_ZEROCOPY");
> > + }
> > + return -1;
> > + }
> > +
> > + sioc->zerocopy_queued++;
> > + return ret;
> > +}
>
> --
> Peter Xu
>
Best regards,
Leonardo Bras
