Re: [PATCH v1 1/3] io: Enable write flags for QIOChannel
Thanks for the feedback Eric!
On Wed, Sep 1, 2021 at 5:54 PM Eric Blake wrote:
>
> 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).
That's interesting!
We could use this patchset for testing that out, as I believe it's easy
to add those flags to the sendmsg() we want to have it enabled.
>
> >
> > Signed-off-by: Leonardo Bras
> > ---
> > 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, , 1,
> > -fds, nfds, NULL);
> > +fds, 0, nfds, NULL);
>
> 0 before nfds here...
Good catch! I will fix that for v2!
>
> > 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.
I kept questioning myself if I was breaking something here :)
>
> ...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.
Yeap!
>
> (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).
Nice tip! just added to my qemu gitconfig :)
>
> > 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
Re: [PATCH v1 1/3] io: Enable write flags for QIOChannel
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
> ---
> 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, , 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 @@
[PATCH v1 1/3] io: Enable write flags for QIOChannel
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.
Signed-off-by: Leonardo Bras
---
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, , 1,
-fds, nfds, NULL);
+fds, 0, nfds, NULL);
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)) {
ret = true;
} else {
trace_mpqemu_send_io_error(msg->cmd, msg->size, nfds);
diff --git a/include/io/channel.h b/include/io/channel.h
index 88988979f8..dada9ebaaf 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -104,6 +104,7 @@ struct QIOChannelClass {
size_t niov,
int *fds,
size_t nfds,
+ int flags,
Error **errp);
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);
+
+#define qio_channel_writev_all(ioc, iov, niov, errp) \
+qio_channel_writev_all_flags(ioc, iov, niov, 0, errp)
/**
* qio_channel_readv:
@@ -364,15 +371,21 @@ ssize_t qio_channel_readv(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
*
* Behaves as qio_channel_writev_full() but does not support
* sending of file handles.
*/
-ssize_t qio_channel_writev(QIOChannel *ioc,
- const struct iovec *iov,
- size_t niov,
- Error **errp);
+ssize_t qio_channel_writev_flags(QIOChannel *ioc,
+ const struct iovec *iov,
+ size_t niov,
+ int flags,
+ Error **errp);
+
+#define qio_channel_writev(ioc, iov, niov, errp) \
+qio_channel_writev_flags(ioc, iov, niov, 0, errp)
+
/**
* qio_channel_read:
@@ -395,16 +408,21 @@ ssize_t qio_channel_read(QIOChannel *ioc,
* @ioc: the channel object
* @buf: the memory regions to send data from
