Re: [PATCH v6 1/6] QIOChannel: Add io_writev_zero_copy & io_flush_zero_copy callbacks

[Daniel P . Berrangé]

On Thu, Dec 09, 2021 at 06:39:19AM -0300, Leonardo Bras wrote:
> Adds io_writev_zero_copy and io_flush_zero_copy as optional callback to 
> QIOChannelClass,
> allowing the implementation of zero copy writes by subclasses.
> 
> How to use them:
> - Write data using qio_channel_writev_zero_copy(),
> - Wait write completion with qio_channel_flush_zero_copy().
> 
> Notes:
> As some zero copy implementations work asynchronously, it's
> recommended to keep the write buffer untouched until the return of
> qio_channel_flush_zero_copy(), to avoid 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_writev_zero_copy will return -1,
> - io_flush_zero_copy 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 <leobras@redhat.com>
> ---
>  include/io/channel.h | 98 +++++++++++++++++++++++++++++++++++++++-----
>  io/channel.c         | 66 +++++++++++++++++++++++------
>  2 files changed, 142 insertions(+), 22 deletions(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 88988979f8..83fa970a19 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,
>  };
>  
>  
> @@ -136,6 +139,12 @@ struct QIOChannelClass {
>                                    IOHandler *io_read,
>                                    IOHandler *io_write,
>                                    void *opaque);
> +    ssize_t (*io_writev_zero_copy)(QIOChannel *ioc,
> +                                   const struct iovec *iov,
> +                                   size_t niov,
> +                                   Error **errp);
> +    int (*io_flush_zero_copy)(QIOChannel *ioc,
> +                              Error **errp);
>  };

I've still got the same feedback as previous iterations. It
does not make sense to having both separate callbacks / APIs
and also add flags to existing methods. It just solves thue
same problem twice which si redundant.

I had suggested separate callbacks originally because I
thought we would need to have different signature with
ability to get completions. We've done completions with
a separate API call though.

So the separate zero_copy methods aren't so compelling
as an idea, and we could just use flags only in
retrospect.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|