| Message ID | 20211009075612.230283-2-leobras@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MSG_ZEROCOPY for multifd | expand |
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 <leobras@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 >
Hello Eric, thank you for the feedback! On Mon, Oct 11, 2021 at 4:17 PM Eric Blake <eblake@redhat.com> wrote: > > 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? I am sorry, I think I missed some renaming before generating the patchset. As you suggested those names are incorrect (as they reflect a previous naming used in v3). I will replace them for io_{writev,flush}_zerocopy in v5. > > > allowing the implementation of asynchronous writes by subclasses. > > > > How to use them: > > - Write data using qio_channel_writev_zerocopu(), > > s/copu/copy/ Thanks, I will fix that. > > > - 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? They are not, I will replace them for io_{writev,flush}_zerocopy in v5. > > > > > 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 <leobras@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. Thanks, I will fix that for v5. > > > }; > > > > /* 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. That makes sense, even though that may be true for just some implementations, it makes sense to document it here. > > > 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. Ok :) > > > + * > > * 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. Ok, Is it enough to document it in a single one of the places suggested, or would you recommend documenting it in all suggested places? > > > + * > > + * If at some point it's necessary wait for all data to be > > s/wait/to wait/ I will fix that for v5, thanks! > > > + * 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/ Yeah, I should have fixed it in v4. Thanks for pointing this out. > > > + * 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? The point here is that any valid user of zrocopy_flush would have already used zerocopy_writev at some point, and failed if not supported / enabled. Having this not returning error can help the user keep a simpler approach when using a setup in which the writev can happen in both zerocopy or default behavior. I mean, the user will not need to check if zerocopy was or was not enabled, and just flush anyway. But if it's not good behavior, or you guys think it's a better approach to fail here, I can also do that. > > > + } > > + > > + 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 > Thank you for reviewing! Best regards, Leo
On Mon, Oct 11, 2021 at 04:38:23PM -0300, Leonardo Bras Soares Passos wrote: > > > /** > > > - * 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. > > That makes sense, even though that may be true for just some implementations, > it makes sense to document it here. > > > Ok, > Is it enough to document it in a single one of the places suggested, or > would you recommend documenting it in all suggested places? Ah, the curse of maintaining copy-and-paste. If you can find a way to say "see this other type for limitations" that sounds fine, it avoids the risk of later edits touching one but not all identical copies. But our current process for generating sphynx documentation from the qapi generator does not have cross-referencing abilities that I'm aware of. Markus or John, any thoughts? > > > > > +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? > > The point here is that any valid user of zrocopy_flush would have > already used zerocopy_writev > at some point, and failed if not supported / enabled. > > Having this not returning error can help the user keep a simpler > approach when using > a setup in which the writev can happen in both zerocopy or default behavior. > > I mean, the user will not need to check if zerocopy was or was not > enabled, and just flush anyway. > > But if it's not good behavior, or you guys think it's a better > approach to fail here, I can also do that. Either flush is supposed to be a no-op when zerocopy fails (so returning 0 is okay), or should not be attempted unless zerocopy succeeded (in which case abort()ing seems like the best way to point out the programmer's error). But I wasn't clear from your documentation which of the two behaviors you had in mind.
On Mon, Oct 11, 2021 at 5:45 PM Eric Blake <eblake@redhat.com> wrote: > > On Mon, Oct 11, 2021 at 04:38:23PM -0300, Leonardo Bras Soares Passos wrote: > > > > /** > > > > - * 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. > > > > That makes sense, even though that may be true for just some implementations, > > it makes sense to document it here. > > > > > > > Ok, > > Is it enough to document it in a single one of the places suggested, or > > would you recommend documenting it in all suggested places? > > Ah, the curse of maintaining copy-and-paste. If you can find a way to > say "see this other type for limitations" that sounds fine, it avoids > the risk of later edits touching one but not all identical copies. > But our current process for generating sphynx documentation from the > qapi generator does not have cross-referencing abilities that I'm > aware of. Markus or John, any thoughts? > > > > > > > > +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? > > > > The point here is that any valid user of zrocopy_flush would have > > already used zerocopy_writev > > at some point, and failed if not supported / enabled. > > > > Having this not returning error can help the user keep a simpler > > approach when using > > a setup in which the writev can happen in both zerocopy or default behavior. > > > > I mean, the user will not need to check if zerocopy was or was not > > enabled, and just flush anyway. > > > > But if it's not good behavior, or you guys think it's a better > > approach to fail here, I can also do that. > > Either flush is supposed to be a no-op when zerocopy fails (so > returning 0 is okay), or should not be attempted unless zerocopy > succeeded (in which case abort()ing seems like the best way to point > out the programmer's error). But I wasn't clear from your > documentation which of the two behaviors you had in mind. Oh, sorry about that. Yeah, I intend to use it as a no-op. If it's fine I will update the docs for v5. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > Thanks!
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. > 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 >
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
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
On Wed, Oct 27, 2021 at 03:07:13AM -0300, Leonardo Bras Soares Passos wrote: > > > > > > if (flags & ZEROCOPY) { > > > assert(fds == NULL && nfds == 0); > > Quick question: Why is this assert needed? Not required I think; just want to make sure no one passes in the fds when using zero copy mode. Thanks,
On Wed, Oct 27, 2021 at 3:15 AM Peter Xu <peterx@redhat.com> wrote: > > On Wed, Oct 27, 2021 at 03:07:13AM -0300, Leonardo Bras Soares Passos wrote: > > > > > > > > if (flags & ZEROCOPY) { > > > > assert(fds == NULL && nfds == 0); > > > > Quick question: Why is this assert needed? > > Not required I think; just want to make sure no one passes in the fds when > using zero copy mode. Ok, that makes sense. I will add it then, thanks! > > Thanks, > > -- > Peter Xu > Best regards, Leo
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); }; /* 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. + * * If there are file descriptors to send, the @fds * array should be non-NULL and provide the handles. * All file descriptors will be sent if at least one @@ -255,12 +269,15 @@ 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) /** * qio_channel_readv_all_eof: @@ -321,10 +338,11 @@ int qio_channel_readv_all(QIOChannel *ioc, /** - * qio_channel_writev_all: + * qio_channel_writev_all_flags: * @ioc: the channel object * @iov: the array of memory regions to write data from * @niov: the length of the @iov array + * @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 @@ -339,10 +357,13 @@ int qio_channel_readv_all(QIOChannel *ioc, * * Returns: 0 if all bytes were written, or -1 on error */ -int qio_channel_writev_all(QIOChannel *ioc, - const struct iovec *iov, - size_t niov, - Error **erp); +int qio_channel_writev_all_flags(QIOChannel *ioc, + const struct iovec *iov, + size_t niov, + int flags, + Error **errp); +#define qio_channel_writev_all(ioc, iov, niov, errp) \ + qio_channel_writev_all_flags(ioc, iov, niov, 0, errp) /** * qio_channel_readv: @@ -831,12 +852,13 @@ int qio_channel_readv_full_all(QIOChannel *ioc, 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. + * * 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. + * + * If at some point it's necessary wait for all data to be + * 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 + * 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 @@ -67,15 +67,27 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, } -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) { QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); + if (flags & QIO_CHANNEL_WRITE_FLAG_ZEROCOPY) { + if (!klass->io_writev_zerocopy || + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY)) { + error_setg_errno(errp, EINVAL, + "Channel does not support zerocopy writev"); + return -1; + } + + return klass->io_writev_zerocopy(ioc, iov, niov, errp); + } + if ((fds || nfds) && !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { error_setg_errno(errp, EINVAL, @@ -212,19 +224,20 @@ int qio_channel_readv_full_all(QIOChannel *ioc, return ret; } -int qio_channel_writev_all(QIOChannel *ioc, - const struct iovec *iov, - size_t niov, - Error **errp) +int qio_channel_writev_all_flags(QIOChannel *ioc, + const struct iovec *iov, + size_t niov, + int flags, + Error **errp) { - return qio_channel_writev_full_all(ioc, iov, niov, NULL, 0, errp); + return qio_channel_writev_full_all_flags(ioc, iov, niov, NULL, 0, flags, errp); } -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); 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); +} + + +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;
Adds io_async_writev and io_async_flush as optional callback to QIOChannelClass, allowing the implementation of asynchronous writes by subclasses. How to use them: - Write data using qio_channel_writev_zerocopu(), - 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 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. 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 <leobras@redhat.com> --- include/io/channel.h | 103 +++++++++++++++++++++++++++++++++++-------- io/channel.c | 74 +++++++++++++++++++++++-------- 2 files changed, 141 insertions(+), 36 deletions(-)