[v3] Zero copy improvements (QIOChannel + multifd)

[PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Leonardo Bras 3 years, 7 months ago

If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
returns 1. This return code should be used only when Linux fails to use
MSG_ZEROCOPY on a lot of sendmsg().

Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
was attempted.

Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 io/channel-socket.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 4466bb1cd4..698c086b70 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
     struct cmsghdr *cm;
     char control[CMSG_SPACE(sizeof(*serr))];
     int received;
-    int ret = 1;
+    int ret;
+
+    if (!sioc->zero_copy_queued) {
+        return 0;
+    }
 
     msg.msg_control = control;
     msg.msg_controllen = sizeof(control);
     memset(control, 0, sizeof(control));
 
+    ret = 1;
+
     while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
         received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
         if (received < 0) {
-- 
2.36.1

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Peter Xu 3 years, 7 months ago

Hi, Leo,

On Mon, Jul 04, 2022 at 05:23:13PM -0300, Leonardo Bras wrote:
> If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
> returns 1. This return code should be used only when Linux fails to use
> MSG_ZEROCOPY on a lot of sendmsg().
> 
> Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
> was attempted.
> 
> Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  io/channel-socket.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 4466bb1cd4..698c086b70 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
>      struct cmsghdr *cm;
>      char control[CMSG_SPACE(sizeof(*serr))];
>      int received;
> -    int ret = 1;
> +    int ret;
> +
> +    if (!sioc->zero_copy_queued) {

I think I asked this in the downstream review but didn't get a
response.. shouldn't this check be "queued == sent"?

> +        return 0;
> +    }
>  
>      msg.msg_control = control;
>      msg.msg_controllen = sizeof(control);
>      memset(control, 0, sizeof(control));
>  
> +    ret = 1;
> +
>      while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
>          received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
>          if (received < 0) {
> -- 
> 2.36.1
> 

-- 
Peter Xu

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Leonardo Bras Soares Passos 3 years, 7 months ago

Hello Peter,

On Thu, Jul 7, 2022 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Leo,
>
> On Mon, Jul 04, 2022 at 05:23:13PM -0300, Leonardo Bras wrote:
> > If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
> > returns 1. This return code should be used only when Linux fails to use
> > MSG_ZEROCOPY on a lot of sendmsg().
> >
> > Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
> > was attempted.
> >
> > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  io/channel-socket.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index 4466bb1cd4..698c086b70 100644
> > --- a/io/channel-socket.c
> > +++ b/io/channel-socket.c
> > @@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
> >      struct cmsghdr *cm;
> >      char control[CMSG_SPACE(sizeof(*serr))];
> >      int received;
> > -    int ret = 1;
> > +    int ret;
> > +
> > +    if (!sioc->zero_copy_queued) {
>
> I think I asked this in the downstream review but didn't get a
> response.. shouldn't this check be "queued == sent"?

This is just supposed to skip flush if nothing was queued for sending.
queued == sent is tested bellow in the while part.

Without this, the function could return 1 if nothing was sent with zero-copy,
and it would be confusing, because the QIOChannel API says 1 should be
returned only if all zero-copy sends fell back to copying.

Best regards,
Leo

>
> > +        return 0;
> > +    }
> >
> >      msg.msg_control = control;
> >      msg.msg_controllen = sizeof(control);
> >      memset(control, 0, sizeof(control));
> >
> > +    ret = 1;
> > +
> >      while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
> >          received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
> >          if (received < 0) {
> > --
> > 2.36.1
> >
>
> --
> Peter Xu
>

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Peter Xu 3 years, 7 months ago

On Thu, Jul 07, 2022 at 04:44:21PM -0300, Leonardo Bras Soares Passos wrote:
> Hello Peter,
> 
> On Thu, Jul 7, 2022 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Leo,
> >
> > On Mon, Jul 04, 2022 at 05:23:13PM -0300, Leonardo Bras wrote:
> > > If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
> > > returns 1. This return code should be used only when Linux fails to use
> > > MSG_ZEROCOPY on a lot of sendmsg().
> > >
> > > Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
> > > was attempted.
> > >
> > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
> > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > ---
> > >  io/channel-socket.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > index 4466bb1cd4..698c086b70 100644
> > > --- a/io/channel-socket.c
> > > +++ b/io/channel-socket.c
> > > @@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
> > >      struct cmsghdr *cm;
> > >      char control[CMSG_SPACE(sizeof(*serr))];
> > >      int received;
> > > -    int ret = 1;
> > > +    int ret;
> > > +
> > > +    if (!sioc->zero_copy_queued) {
> >
> > I think I asked this in the downstream review but didn't get a
> > response.. shouldn't this check be "queued == sent"?
> 
> This is just supposed to skip flush if nothing was queued for sending.
> queued == sent is tested bellow in the while part.
> 
> Without this, the function could return 1 if nothing was sent with zero-copy,
> and it would be confusing, because the QIOChannel API says 1 should be
> returned only if all zero-copy sends fell back to copying.

I know it won't happen in practise, but.. what if we call flush() without
sending anything zero-copy-wise at all (so zero_copy_sent > 0,
zero_copy_queued > 0, meanwhile zero_copy_sent == zero_copy_queued)?  Then
IIUC we'll return 1 even if we didn't do any fallback, or am I wrong?

> 
> Best regards,
> Leo
> 
> >
> > > +        return 0;
> > > +    }
> > >
> > >      msg.msg_control = control;
> > >      msg.msg_controllen = sizeof(control);
> > >      memset(control, 0, sizeof(control));
> > >
> > > +    ret = 1;
> > > +
> > >      while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
> > >          received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
> > >          if (received < 0) {
> > > --
> > > 2.36.1
> > >
> >
> > --
> > Peter Xu
> >
> 

-- 
Peter Xu

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Leonardo Brás 3 years, 7 months ago

On Thu, 2022-07-07 at 15:52 -0400, Peter Xu wrote:
> On Thu, Jul 07, 2022 at 04:44:21PM -0300, Leonardo Bras Soares Passos wrote:
> > Hello Peter,
> > 
> > On Thu, Jul 7, 2022 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
> > > 
> > > Hi, Leo,
> > > 
> > > On Mon, Jul 04, 2022 at 05:23:13PM -0300, Leonardo Bras wrote:
> > > > If flush is called when no buffer was sent with MSG_ZEROCOPY, it
> > > > currently
> > > > returns 1. This return code should be used only when Linux fails to use
> > > > MSG_ZEROCOPY on a lot of sendmsg().
> > > > 
> > > > Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
> > > > was attempted.
> > > > 
> > > > Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy
> > > > flag & io_flush for CONFIG_LINUX")
> > > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > > ---
> > > >  io/channel-socket.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > > index 4466bb1cd4..698c086b70 100644
> > > > --- a/io/channel-socket.c
> > > > +++ b/io/channel-socket.c
> > > > @@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel
> > > > *ioc,
> > > >      struct cmsghdr *cm;
> > > >      char control[CMSG_SPACE(sizeof(*serr))];
> > > >      int received;
> > > > -    int ret = 1;
> > > > +    int ret;
> > > > +
> > > > +    if (!sioc->zero_copy_queued) {
> > > 
> > > I think I asked this in the downstream review but didn't get a
> > > response.. shouldn't this check be "queued == sent"?
> > 
> > This is just supposed to skip flush if nothing was queued for sending.
> > queued == sent is tested bellow in the while part.
> > 
> > Without this, the function could return 1 if nothing was sent with zero-
> > copy,
> > and it would be confusing, because the QIOChannel API says 1 should be
> > returned only if all zero-copy sends fell back to copying.
> 
> I know it won't happen in practise, but.. what if we call flush() without
> sending anything zero-copy-wise at all (so zero_copy_sent > 0,
> zero_copy_queued > 0, 

On a no-bug scenario:
- If we don't send anything zero-copy wise, zero_copy_queued will never get
incremented. 
- If we don't get inside the while loop in flush, zero_copy_sent will never be
incremented.


But sure, suppose it does get incremented by bug / mistake: 
-  zero_copy_queued incremented, zero_copy_sent untouched : 
 On (queued == 0) it will go past the 'if', and get stuck 'forever' in the while
loop, since sent will 'never' get incremented.
 On (queued == sent), same.
 On (queued <= sent), same.

-  zero_copy_queued untouched, zero_copy_sent incremented :
 On 'if (queued == 0)' it will not go past the 'if'
 On 'if (queued == sent)', will go past the 'if', but will exit on the 'while',
falsely returning 1.
 On 'if (queued <= sent)', it will not go past the 'if'.

-  zero_copy_queued incremented, zero_copy_sent incremented : 
 On 'if (queued == 0)',  infinite loop as above.
 On 'if (queued == sent)', 
	if sent < queued :  infinite loop , 
	if sent == queued : won't go past 'if' ,
       	if sent > queued :  will go past the 'if', but will exit on the
'while', falsely returning 1.  
 On (queued <= sent), infinite loop if sent < queued, not past if otherwise

BTW, I consider it 'infinite loop', but it could also end up returning -1 on any
error.

> meanwhile zero_copy_sent == zero_copy_queued)?  Then
> IIUC we'll return 1 even if we didn't do any fallback, or am I wrong?

Having 'if(queued == sent)' will cause us to falsely return '1' in two buggy
cases, while 'if queued == 0) will either skip early or go into 'infinite' loop.

Best regards,
Leo

> 
> > 
> > Best regards,
> > Leo
> > 
> > > 
> > > > +        return 0;
> > > > +    }
> > > > 
> > > >      msg.msg_control = control;
> > > >      msg.msg_controllen = sizeof(control);
> > > >      memset(control, 0, sizeof(control));
> > > > 
> > > > +    ret = 1;
> > > > +
> > > >      while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
> > > >          received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
> > > >          if (received < 0) {
> > > > --
> > > > 2.36.1
> > > > 
> > > 
> > > --
> > > Peter Xu
> > > 
> > 
>

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Peter Xu 3 years, 7 months ago

On Thu, Jul 07, 2022 at 06:14:17PM -0300, Leonardo Brás wrote:
> Having 'if(queued == sent)' will cause us to falsely return '1' in two buggy
> cases, while 'if queued == 0) will either skip early or go into 'infinite' loop.

I'm not sure I strictly follow here..

Imagine the case we do flush() twice without sending anything, then in the
1st flush we'll see queued>sent, we'll finish flush() until queued==sent.
Then in the 2nd (continuous) flush() we'll see queued==sent immediately.

IIUC with the current patch we'll return 1 which I think is wrong because
fallback didn't happen, and if with the change to "if (queued==sent) return
0" it'll fix it?

-- 
Peter Xu

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Leonardo Bras Soares Passos 3 years, 7 months ago

On Thu, Jul 7, 2022 at 7:18 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Jul 07, 2022 at 06:14:17PM -0300, Leonardo Brás wrote:
> > Having 'if(queued == sent)' will cause us to falsely return '1' in two buggy
> > cases, while 'if queued == 0) will either skip early or go into 'infinite' loop.
>
> I'm not sure I strictly follow here..
>

Sorry, I was thinking of a different scenario.

> Imagine the case we do flush() twice without sending anything, then in the
> 1st flush we'll see queued>sent, we'll finish flush() until queued==sent.
> Then in the 2nd (continuous) flush() we'll see queued==sent immediately.
>
> IIUC with the current patch we'll return 1 which I think is wrong because
> fallback didn't happen, and if with the change to "if (queued==sent) return
> 0" it'll fix it?

Yes, you are correct.
It's a possible scenario to have a flush happen just after another
without any sending in between.

I will fix it as suggested.

Best regards,
Leo

>
> --
> Peter Xu
>

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Daniel P. Berrangé 3 years, 7 months ago

On Mon, Jul 04, 2022 at 05:23:13PM -0300, Leonardo Bras wrote:
> If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
> returns 1. This return code should be used only when Linux fails to use
> MSG_ZEROCOPY on a lot of sendmsg().
> 
> Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
> was attempted.
> 
> Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  io/channel-socket.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

And if this merges via migration maintainers' tree

Acked-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 4466bb1cd4..698c086b70 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
>      struct cmsghdr *cm;
>      char control[CMSG_SPACE(sizeof(*serr))];
>      int received;
> -    int ret = 1;
> +    int ret;
> +
> +    if (!sioc->zero_copy_queued) {
> +        return 0;
> +    }
>  
>      msg.msg_control = control;
>      msg.msg_controllen = sizeof(control);
>      memset(control, 0, sizeof(control));
>  
> +    ret = 1;
> +
>      while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
>          received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
>          if (received < 0) {
> -- 
> 2.36.1
> 

With 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 :|

Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

Posted by Juan Quintela 3 years, 7 months ago

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Jul 04, 2022 at 05:23:13PM -0300, Leonardo Bras wrote:
>> If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
>> returns 1. This return code should be used only when Linux fails to use
>> MSG_ZEROCOPY on a lot of sendmsg().
>> 
>> Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
>> was attempted.
>> 
>> Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
>> Signed-off-by: Leonardo Bras <leobras@redhat.com>
>> ---
>>  io/channel-socket.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> And if this merges via migration maintainers' tree
>
> Acked-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I will add this on the next PULLL request.

Thanks.


>> 
>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> index 4466bb1cd4..698c086b70 100644
>> --- a/io/channel-socket.c
>> +++ b/io/channel-socket.c
>> @@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
>>      struct cmsghdr *cm;
>>      char control[CMSG_SPACE(sizeof(*serr))];
>>      int received;
>> -    int ret = 1;
>> +    int ret;
>> +
>> +    if (!sioc->zero_copy_queued) {
>> +        return 0;
>> +    }
>>  
>>      msg.msg_control = control;
>>      msg.msg_controllen = sizeof(control);
>>      memset(control, 0, sizeof(control));
>>  
>> +    ret = 1;
>> +
>>      while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
>>          received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
>>          if (received < 0) {
>> -- 
>> 2.36.1
>> 
>
> With regards,
> Daniel