Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent
On Thu, Jul 7, 2022 at 7:18 PM Peter Xu 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
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
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 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 > > > > --- > > > > 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_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
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 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 > > > --- > > > 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_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
Hello Peter, On Thu, Jul 7, 2022 at 2:47 PM Peter Xu 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 > > --- > > 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_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
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 > --- > 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_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
Daniel P. Berrangé 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 >> --- >> io/channel-socket.c | 8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) > > Reviewed-by: Daniel P. Berrangé > > And if this merges via migration maintainers' tree > > Acked-by: Daniel P. Berrangé Reviewed-by: Juan Quintela 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_ERRQUEUE); >> if (received < 0) { >> -- >> 2.36.1 >> > > With regards, > Daniel
Re: [PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent
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 > --- > io/channel-socket.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé And if this merges via migration maintainers' tree Acked-by: Daniel P. Berrangé > > 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_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 :|
[PATCH v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent
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 --- 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_ERRQUEUE); if (received < 0) { -- 2.36.1