Hello Daniel,
On Thu, Jan 13, 2022 at 7:53 AM Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> On Thu, Jan 06, 2022 at 07:13:38PM -0300, Leonardo Bras wrote:
> > Add flags to io_writev and introduce io_flush as optional callback to
> > QIOChannelClass, allowing the implementation of zero copy writes by
> > subclasses.
> >
> > How to use them:
> > - Write data using qio_channel_writev(...,QIO_CHANNEL_WRITE_FLAG_ZERO_COPY),
> > - Wait write completion with qio_channel_flush().
> >
> > Notes:
> > As some zero copy implementations work asynchronously, it's
> > recommended to keep the write buffer untouched until the return of
> > qio_channel_flush(), to avoid the risk of sending an updated buffer
> > instead of the buffer state during write.
> >
> > As io_flush callback is optional, if a subclass does not implement it, then:
> > - io_flush will return 0 without changing anything.
> >
> > Also, some functions like qio_channel_writev_full_all() were adapted to
> > receive a flag parameter. That allows shared code between zero copy and
> > non-zero copy writev, and also an easier implementation on new flags.
> >
> > Signed-off-by: Leonardo Bras <leob...@redhat.com>
> > ---
> > include/io/channel.h | 67 +++++++++++++++++++++++++++++++++++---------
> > io/channel-buffer.c | 1 +
> > io/channel-command.c | 1 +
> > io/channel-file.c | 1 +
> > io/channel-socket.c | 2 ++
> > io/channel-tls.c | 1 +
> > io/channel-websock.c | 1 +
> > io/channel.c | 51 +++++++++++++++++++++++----------
> > migration/rdma.c | 1 +
> > 9 files changed, 98 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/io/channel.h b/include/io/channel.h
> > index 88988979f8..343766ce5b 100644
> > --- a/include/io/channel.h
> > +++ b/include/io/channel.h
> > @@ -32,12 +32,15 @@ OBJECT_DECLARE_TYPE(QIOChannel, QIOChannelClass,
> >
> > #define QIO_CHANNEL_ERR_BLOCK -2
> >
> > +#define QIO_CHANNEL_WRITE_FLAG_ZERO_COPY 0x1
> > +
> > typedef enum QIOChannelFeature QIOChannelFeature;
> >
> > enum QIOChannelFeature {
> > QIO_CHANNEL_FEATURE_FD_PASS,
> > QIO_CHANNEL_FEATURE_SHUTDOWN,
> > QIO_CHANNEL_FEATURE_LISTEN,
> > + QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY,
> > };
> >
> >
> > @@ -104,6 +107,7 @@ struct QIOChannelClass {
> > size_t niov,
> > int *fds,
> > size_t nfds,
> > + int flags,
> > Error **errp);
> > ssize_t (*io_readv)(QIOChannel *ioc,
> > const struct iovec *iov,
> > @@ -136,6 +140,8 @@ struct QIOChannelClass {
> > IOHandler *io_read,
> > IOHandler *io_write,
> > void *opaque);
> > + int (*io_flush)(QIOChannel *ioc,
> > + Error **errp);
> > };
> >
> > /* General I/O handling functions */
> > @@ -222,12 +228,13 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
> >
> >
> > /**
> > - * qio_channel_writev_full:
> > + * qio_channel_writev_full_flags:
> > * @ioc: the channel object
> > * @iov: the array of memory regions to write data from
> > * @niov: the length of the @iov array
> > * @fds: an array of file handles to send
> > * @nfds: number of file handles in @fds
> > + * @flags: write flags (QIO_CHANNEL_WRITE_FLAG_*)
> > * @errp: pointer to a NULL-initialized error object
> > *
> > * Write data to the IO channel, reading it from the
> > @@ -255,12 +262,16 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
> > * or QIO_CHANNEL_ERR_BLOCK if no data is can be sent
> > * and the channel is non-blocking
> > */
> > -ssize_t qio_channel_writev_full(QIOChannel *ioc,
> > - const struct iovec *iov,
> > - size_t niov,
> > - int *fds,
> > - size_t nfds,
> > - Error **errp);
> > +ssize_t qio_channel_writev_full_flags(QIOChannel *ioc,
> > + const struct iovec *iov,
> > + size_t niov,
> > + int *fds,
> > + size_t nfds,
> > + int flags,
> > + Error **errp);
> > +
> > +#define qio_channel_writev_full(ioc, iov, niov, fds, nfds, errp) \
> > + qio_channel_writev_full_flags(ioc, iov, niov, fds, nfds, 0, errp)
>
> Don't introduce yet another API variant here. Just add flags to
> all the existing write APIs with "full" in their name. The word
> "full" in their name was intended to indicate that they are
> accepting all possible parameters, so it doesn't mean sense to
> add APIs which take even more possible parameters.
Oh, I was not aware of that. Thanks for letting me know!
Sure, I will do this change for v8.
>
> > +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);
> > +#define qio_channel_writev_full_all(ioc, iov, niov, fds, nfds, errp) \
> > + qio_channel_writev_full_all_flags(ioc, iov, niov, fds, nfds, 0, errp)
>
> Same note.
>
> > +/**
> > + * qio_channel_flush:
> > + * @ioc: the channel object
> > + * @errp: pointer to a NULL-initialized error object
> > + *
> > + * Will block until every packet queued with
> > + * qio_channel_writev_full_flags() + QIO_CHANNEL_WRITE_FLAG_ZERO_COPY
> > + * is sent, or return in case of any error.
> > + *
> > + * If not implemented, acts as a no-op, and returns 0.
> > + *
> > + * Returns -1 if any error is found,
> > + * 1 if every send failed to use zero copy.
> > + * 0 otherwise.
> > + */
> > +
> > +int qio_channel_flush(QIOChannel *ioc,
> > + Error **errp);
>
>
Best regards,
Leo
