On Sat, Oct 09, 2021 at 04:56:11AM -0300, Leonardo Bras wrote: > Adds io_async_writev and io_async_flush as optional callback to > QIOChannelClass,
Are these names accurate? > allowing the implementation of asynchronous writes by subclasses. > > How to use them: > - Write data using qio_channel_writev_zerocopu(), s/copu/copy/ > - Wait write completion with qio_channel_flush_zerocopy(). > > Notes: > As some zerocopy implementations work asynchronously, it's > recommended to keep the write buffer untouched until the return of > qio_channel_flush_zerocopy(), by the risk of sending an updated buffer s/by/to avoid/ > instead of the one at the write. > > As the new callbacks are optional, if a subclass does not implement them, > then: > - io_async_writev will return -1, > - io_async_flush will return 0 without changing anything. Are these names accurate? > > Also, some functions like qio_channel_writev_full_all() were adapted to > receive a flag parameter. That allows shared code between zerocopy and > non-zerocopy writev. > > Signed-off-by: Leonardo Bras <leob...@redhat.com> > --- > include/io/channel.h | 103 +++++++++++++++++++++++++++++++++++-------- > io/channel.c | 74 +++++++++++++++++++++++-------- > 2 files changed, 141 insertions(+), 36 deletions(-) > > diff --git a/include/io/channel.h b/include/io/channel.h > index 88988979f8..e7d4e1521f 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_ZEROCOPY 0x1 > + > typedef enum QIOChannelFeature QIOChannelFeature; > > enum QIOChannelFeature { > QIO_CHANNEL_FEATURE_FD_PASS, > QIO_CHANNEL_FEATURE_SHUTDOWN, > QIO_CHANNEL_FEATURE_LISTEN, > + QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY, > }; > > > @@ -136,6 +139,12 @@ struct QIOChannelClass { > IOHandler *io_read, > IOHandler *io_write, > void *opaque); > + ssize_t (*io_writev_zerocopy)(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + Error **errp); > + int (*io_flush_zerocopy)(QIOChannel *ioc, > + Error **errp); Indentation is off by one. > }; > > /* General I/O handling functions */ > @@ -222,12 +231,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 > @@ -242,6 +252,10 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, > * guaranteed. If the channel is non-blocking and no > * data can be sent, it will return QIO_CHANNEL_ERR_BLOCK > * > + * If flag QIO_CHANNEL_WRITE_FLAG_ZEROCOPY is passed, > + * function will return once each buffer was queued for > + * sending. This would be a good place to document the requirement to keep the buffer unchanged until the zerocopy sequence completes. > Error **errp); > > /** > - * qio_channel_writev_full_all: > + * qio_channel_writev_full_all_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 > * > * > @@ -846,13 +868,58 @@ int qio_channel_readv_full_all(QIOChannel *ioc, > * to be written, yielding from the current coroutine > * if required. > * > + * If QIO_CHANNEL_WRITE_FLAG_ZEROCOPY is passed in flags, > + * instead of waiting for all requested data to be written, > + * this function will wait until it's all queued for writing. Another good place to document restrictions on buffer stability. > + * > * Returns: 0 if all bytes were written, or -1 on error > */ > > -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); > +#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) > + > +/** > + * qio_channel_writev_zerocopy: > + * @ioc: the channel object > + * @iov: the array of memory regions to write data from > + * @niov: the length of the @iov array > + * @errp: pointer to a NULL-initialized error object > + * > + * Behaves like qio_channel_writev_full_all_flags, but will write > + * data asynchronously while avoiding unnecessary data copy. > + * This function may return before any data is actually written, > + * but should queue every buffer for writting. writing Another place to document buffer stability considerations. > + * > + * If at some point it's necessary wait for all data to be s/wait/to wait/ > + * written, use qio_channel_flush_zerocopy(). > + * > + * If zerocopy is not available, returns -1 and set errp. > + */ > + > +ssize_t qio_channel_writev_zerocopy(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + Error **errp); > + > +/** > + * qio_channel_flush_zerocopy: > + * @ioc: the channel object > + * @errp: pointer to a NULL-initialized error object > + * > + * Will lock until every packet queued with s/lock/block/ > + * qio_channel_writev_zerocopy() is sent, or return > + * in case of any error. > + * > + * Returns -1 if any error is found, 0 otherwise. > + * If not implemented, returns 0 without changing anything. > + */ > + > +int qio_channel_flush_zerocopy(QIOChannel *ioc, > + Error **errp); > > #endif /* QIO_CHANNEL_H */ > diff --git a/io/channel.c b/io/channel.c > index e8b019dc36..811c93ae23 100644 > --- a/io/channel.c > +++ b/io/channel.c > +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; Matches your documentation, but an ideal app should not be trying to flush if the write failed in the first place. So wouldn't it be better to return -1 or even abort() on a coding error? > + } > + > + return klass->io_flush_zerocopy(ioc, errp); > +} > + > + > static void qio_channel_restart_read(void *opaque) > { > QIOChannel *ioc = opaque; > -- > 2.33.0 > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org