Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks

[Peter Xu <peterx@redhat.com>]

On Wed, Oct 13, 2021 at 02:07:13PM +0800, Peter Xu wrote:
> On Sat, Oct 09, 2021 at 04:56:11AM -0300, Leonardo Bras wrote:
> > -int qio_channel_writev_full_all(QIOChannel *ioc,
> > -                                const struct iovec *iov,
> > -                                size_t niov,
> > -                                int *fds, size_t nfds,
> > -                                Error **errp)
> > +int qio_channel_writev_full_all_flags(QIOChannel *ioc,
> > +                                      const struct iovec *iov,
> > +                                      size_t niov,
> > +                                      int *fds, size_t nfds,
> > +                                      int flags, Error **errp)
> >  {
> >      int ret = -1;
> >      struct iovec *local_iov = g_new(struct iovec, niov);
> > @@ -237,8 +250,8 @@ int qio_channel_writev_full_all(QIOChannel *ioc,
> >  
> >      while (nlocal_iov > 0) {
> >          ssize_t len;
> > -        len = qio_channel_writev_full(ioc, local_iov, nlocal_iov, fds, nfds,
> > -                                      errp);
> > +        len = qio_channel_writev_full_flags(ioc, local_iov, nlocal_iov, fds, nfds,
> > +                                          flags, errp);
> 
> IMHO we can keep qio_channel_writev_full() untouched, as it is the wrapper for
> io_writev() hook right now (and it allows taking fds).  Then here instead of
> adding a new flags into it, we can introduce qio_channel_writev_zerocopy_full()
> and directly call here:
> 
>            if (flags & ZEROCOPY) {
>                assert(fds == NULL && nfds == 0);
>                qio_channel_writev_zerocopy_full(...[no fds passing in]);
>            } else {
>                qio_channel_writev_full(...[with all parameters]);
>            }
> 
> Then qio_channel_writev_zerocopy_full() should be simplely the wrapper for the
> new io_writev_zerocopy() hook.

Sorry I think the name doesn't need to have "_full" - that should be used for
io_writev() when we need to pass in fds.  zerocopy doesn't have that, so I
think we can just call it qio_channel_writev_zerocopy() as it simply does what
writev() does.

> 
> >          if (len == QIO_CHANNEL_ERR_BLOCK) {
> >              if (qemu_in_coroutine()) {
> >                  qio_channel_yield(ioc, G_IO_OUT);
> > @@ -474,6 +487,31 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
> >  }
> >  
> >  
> > +ssize_t qio_channel_writev_zerocopy(QIOChannel *ioc,
> > +                                    const struct iovec *iov,
> > +                                    size_t niov,
> > +                                    Error **errp)
> > +{
> > +    return qio_channel_writev_full_flags(ioc, iov, niov, NULL, 0,
> > +                                         QIO_CHANNEL_WRITE_FLAG_ZEROCOPY,
> > +                                         errp);
> > +}
> 
> This function is never used, right? qio_channel_writev_all_flags() is used in
> patch 3, with proper flags passed in.  Then IMHO we can drop this one.
> 
> The rest looks good, as long as with Eric's comment addressed.
> 
> Thanks,
> 
> > +
> > +
> > +int qio_channel_flush_zerocopy(QIOChannel *ioc,
> > +                               Error **errp)
> > +{
> > +    QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
> > +
> > +    if (!klass->io_flush_zerocopy ||
> > +        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY)) {
> > +        return 0;
> > +    }
> > +
> > +    return klass->io_flush_zerocopy(ioc, errp);
> > +}
> > +
> > +
> >  static void qio_channel_restart_read(void *opaque)
> >  {
> >      QIOChannel *ioc = opaque;
> > -- 
> > 2.33.0
> > 
> 
> -- 
> Peter Xu

-- 
Peter Xu