Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
On Wed, Oct 27, 2021 at 3:15 AM Peter Xu 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
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
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,
--
Peter Xu
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
Hello Peter, sorry for the delay.
On Wed, Oct 13, 2021 at 3:33 AM Peter Xu 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
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
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
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
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
>
--
Peter Xu
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
On Mon, Oct 11, 2021 at 5:45 PM Eric Blake 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!
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
Hello Eric, thank you for the feedback!
On Mon, Oct 11, 2021 at 4:17 PM Eric Blake 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
> > ---
> > 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,
Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
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
> ---
> 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
