Re: [PATCH v1 1/3] io: Enable write flags for QIOChannel

[Eric Blake]

On Tue, Aug 31, 2021 at 08:02:37AM -0300, Leonardo Bras wrote:
> Some syscalls used for writting, such as sendmsg(), accept flags that
> can modify their behavior, even allowing the usage of features such as
> MSG_ZEROCOPY.
> 
> Change qio_channel_write*() interface to allow passing down flags,
> allowing a more flexible use of IOChannel.
> 
> At first, it's use is enabled for QIOChannelSocket, but can be easily
> extended to any other QIOChannel implementation.

As a followup to this patch, I wonder if we can also get performance
improvements by implementing MSG_MORE, and using that in preference to
corking/uncorking to better indicate that back-to-back short messages
may behave better when grouped together over the wire.  At least the
NBD code could make use of it (going off of my experience with the
libnbd project demonstrating a performance boost when we added
MSG_MORE support there).

> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  chardev/char-io.c                   |  2 +-
>  hw/remote/mpqemu-link.c             |  2 +-
>  include/io/channel.h                | 56 ++++++++++++++++++++---------
>  io/channel-buffer.c                 |  1 +
>  io/channel-command.c                |  1 +
>  io/channel-file.c                   |  1 +
>  io/channel-socket.c                 |  4 ++-
>  io/channel-tls.c                    |  1 +
>  io/channel-websock.c                |  1 +
>  io/channel.c                        | 53 ++++++++++++++-------------
>  migration/rdma.c                    |  1 +
>  scsi/pr-manager-helper.c            |  2 +-
>  tests/unit/test-io-channel-socket.c |  1 +
>  13 files changed, 81 insertions(+), 45 deletions(-)
> 
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 8ced184160..4ea7b1ee2a 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -122,7 +122,7 @@ int io_channel_send_full(QIOChannel *ioc,
>  
>          ret = qio_channel_writev_full(
>              ioc, &iov, 1,
> -            fds, nfds, NULL);
> +            fds, 0, nfds, NULL);

0 before nfds here...

>          if (ret == QIO_CHANNEL_ERR_BLOCK) {
>              if (offset) {
>                  return offset;
> diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c
> index 7e841820e5..0d13321ef0 100644
> --- a/hw/remote/mpqemu-link.c
> +++ b/hw/remote/mpqemu-link.c
> @@ -69,7 +69,7 @@ bool mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error 
> **errp)
>      }
>  
>      if (!qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send),
> -                                    fds, nfds, errp)) {
> +                                     fds, nfds, 0, errp)) {

Thanks for fixing the broken indentation.

...but after nfds here, so one is wrong; up to this point in a linear
review, I can't tell which was intended...

> +++ b/include/io/channel.h
> @@ -104,6 +104,7 @@ struct QIOChannelClass {
>                           size_t niov,
>                           int *fds,
>                           size_t nfds,
> +                         int flags,
>                           Error **errp);

...and finally I see that in general, you wanted to add the argument
after.  Looks like the change to char-io.c is buggy.

(You can use scripts/git.orderfile as a way to force your patch to
list the .h file first, to make it easier for linear review).

>      ssize_t (*io_readv)(QIOChannel *ioc,
>                          const struct iovec *iov,
> @@ -260,6 +261,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
>                                  size_t niov,
>                                  int *fds,
>                                  size_t nfds,
> +                                int flags,
>                                  Error **errp);
>  
>  /**
> @@ -325,6 +327,7 @@ int qio_channel_readv_all(QIOChannel *ioc,
>   * @ioc: the channel object
>   * @iov: the array of memory regions to write data from
>   * @niov: the length of the @iov array
> + * @flags: optional sending flags
>   * @errp: pointer to a NULL-initialized error object
>   *
>   * Write data to the IO channel, reading it from the
> @@ -339,10 +342,14 @@ 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);

You changed the function name here, but not in the comment beforehand.

> +
> +#define qio_channel_writev_all(ioc, iov, niov, errp) \
> +    qio_channel_writev_all_flags(ioc, iov, niov, 0, errp)

In most cases, you were merely adding a new function to minimize churn
to existing callers while making the old name a macro,...

> @@ -853,6 +876,7 @@ int qio_channel_writev_full_all(QIOChannel *ioc,
>                                  const struct iovec *iov,
>                                  size_t niov,
>                                  int *fds, size_t nfds,
> +                                int flags,
>                                  Error **errp);

...but this one you changed in-place.  Any reason?  It might be nice
to mention how you chose which functions to wrap (to minimize churn to
existing clients) vs. modify signatures.

>  
>  #endif /* QIO_CHANNEL_H */
> diff --git a/io/channel-buffer.c b/io/channel-buffer.c
> index baa4e2b089..bf52011be2 100644
> --- a/io/channel-buffer.c
> +++ b/io/channel-buffer.c
> @@ -81,6 +81,7 @@ static ssize_t qio_channel_buffer_writev(QIOChannel *ioc,
>                                           size_t niov,
>                                           int *fds,
>                                           size_t nfds,
> +                                         int flags,
>                                           Error **errp)
>  {
>      QIOChannelBuffer *bioc = QIO_CHANNEL_BUFFER(ioc);

Would it be wise to check that flags only contains values we can honor
in this (and all other) implementations of qio backends?  Do we need
some way for a backend to advertise to the core qio code which flags
it is willing to accept?

> +++ b/io/channel-socket.c
> @@ -525,6 +525,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>                                           size_t niov,
>                                           int *fds,
>                                           size_t nfds,
> +                                         int flags,
>                                           Error **errp)
>  {
>      QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> @@ -558,7 +559,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>      }
>  
>   retry:
> -    ret = sendmsg(sioc->fd, &msg, 0);
> +    ret = sendmsg(sioc->fd, &msg, flags);

Because of this line, we are forcing our use of flags to be exactly
the same set of MSG_* flags understood by sendmsg(), which feels a bit
fragile.  Wouldn't it be safer to define our own set of QIO_MSG_
flags, and map those into whatever flag values the underlying backends
can support?  After all, one thing I learned on libnbd is that
MSG_MORE is not universally portable, but the goal of qio is to
abstract away things so that the rest of the code doesn't have to do
#ifdef tests everywhere, but instead let the qio code deal with it
(whether to ignore an unsupported flag because it is only an
optimization hint, or to return an error because we depend on the
behavior change the flag would cause if supported, or...).  And that
goes back to my question of whether backends should have a way to
inform the qio core which flags they can support.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org