[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocop
From: |
Peter Xu |
Subject: |
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks |
Date: |
Wed, 13 Oct 2021 14:32:53 +0800 |
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
[PATCH v4 2/3] QIOChannelSocket: Implement io_writev_zerocopy & io_flush_zerocopy for CONFIG_LINUX, Leonardo Bras, 2021/10/09
[PATCH v4 3/3] multifd: Implement zerocopy write in multifd migration (multifd-zerocopy), Leonardo Bras, 2021/10/09