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

[Leonardo Bras Soares Passos <leobras@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4090 bytes --]

Hello Peter, sorry for the delay.

On Wed, Oct 13, 2021 at 3:33 AM Peter Xu <peterx@redhat.com> wrote:
>
> 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:

Sure, it makes sense.

> >
> >            if (flags & ZEROCOPY) {
> >                assert(fds == NULL && nfds == 0);

Quick question: Why is this assert needed?

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

Ok, I will make these changes to v5.


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

IIUC, this was addressed by your reply above, is that correct?

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

Best regards,
Leonardo Bras

[-- Attachment #2: Type: text/html, Size: 6424 bytes --]