Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Paolo Bonzini]

On 05/06/2018 09:54, Sergio Lopez wrote:
> Only retry on serial_xmit if qemu_chr_fe_write returns 0, as this is the
> only recoverable error.
> 
> Retrying with any other scenario, in addition to being a waste of CPU
> cycles, can compromise the Guest stability if by the vCPU issuing the
> write and the main loop thread are, by chance or explicit pinning,
> running on the same pCPU.
> 
> Previous discussion:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06998.html
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  hw/char/serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 605b0d0..6de6c29 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -260,7 +260,7 @@ static void serial_xmit(SerialState *s)
>          if (s->mcr & UART_MCR_LOOP) {
>              /* in loopback mode, say that we just received a char */
>              serial_receive1(s, &s->tsr, 1);
> -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) != 1 &&
> +        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
>                     s->tsr_retry < MAX_XMIT_RETRY) {
>              assert(s->watch_tag == 0);
>              s->watch_tag =
> 

Queued, thanks to you and all those who participated in the discussion.

Paolo

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Marc-André Lureau]

Hi

On Tue, Jun 5, 2018 at 11:18 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 05/06/2018 09:54, Sergio Lopez wrote:
>> Only retry on serial_xmit if qemu_chr_fe_write returns 0, as this is the
>> only recoverable error.
>>
>> Retrying with any other scenario, in addition to being a waste of CPU
>> cycles, can compromise the Guest stability if by the vCPU issuing the
>> write and the main loop thread are, by chance or explicit pinning,
>> running on the same pCPU.
>>
>> Previous discussion:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06998.html
>>
>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> ---
>>  hw/char/serial.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>> index 605b0d0..6de6c29 100644
>> --- a/hw/char/serial.c
>> +++ b/hw/char/serial.c
>> @@ -260,7 +260,7 @@ static void serial_xmit(SerialState *s)
>>          if (s->mcr & UART_MCR_LOOP) {
>>              /* in loopback mode, say that we just received a char */
>>              serial_receive1(s, &s->tsr, 1);
>> -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) != 1 &&
>> +        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
>>                     s->tsr_retry < MAX_XMIT_RETRY) {
>>              assert(s->watch_tag == 0);
>>              s->watch_tag =
>>
>
> Queued, thanks to you and all those who participated in the discussion.

Shouldn't it rety if it returns -1 and the errno == EAGAIN? I am not
sure when it would return 0 and a retry would make sense.



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Peter Maydell]

On 9 July 2018 at 11:25, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Shouldn't it rety if it returns -1 and the errno == EAGAIN? I am not
> sure when it would return 0 and a retry would make sense.

This came up in previous discussion on an earlier version of this:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg00171.html

qemu_chr_fe_write() should never return -1/EAGAIN, because then
all of its callers need to deal with it. It should handle EAGAIN
internally and report "try again later" by returning 0.

The logic is that (a) EAGAIN is a pain to handle and we should
avoid expanding the set of places in QEMU that has to care about
it where we can and (b) this function already has a way to say
'try again later' (ie, returning 0), so it is confusing and
complicates callsites to give it a second way to say the same
thing (ie, returning -1/EINTR). Better to always report "try
again" in the same way.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Marc-André Lureau]

Hi

On Mon, Jul 9, 2018 at 2:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 July 2018 at 11:25, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>> Shouldn't it rety if it returns -1 and the errno == EAGAIN? I am not
>> sure when it would return 0 and a retry would make sense.
>
> This came up in previous discussion on an earlier version of this:
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg00171.html
>

Thanks, I lost that thread somehow

> qemu_chr_fe_write() should never return -1/EAGAIN, because then
> all of its callers need to deal with it. It should handle EAGAIN
> internally and report "try again later" by returning 0.

But that's not the case, unless qemu_chr_write() 'write_all' arg is true.

qemu_chr_fe_write() uses false.

I suppose the patch should also change qemu_chr_fe_write() to
qemu_chr_fe_write_all() to be complete and let the chardev handle
EAGAIN.



>
> The logic is that (a) EAGAIN is a pain to handle and we should
> avoid expanding the set of places in QEMU that has to care about
> it where we can and (b) this function already has a way to say
> 'try again later' (ie, returning 0), so it is confusing and
> complicates callsites to give it a second way to say the same
> thing (ie, returning -1/EINTR). Better to always report "try
> again" in the same way.
>
> thanks
> -- PMM



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Paolo Bonzini]

On 09/07/2018 14:23, Marc-André Lureau wrote:
> I suppose the patch should also change qemu_chr_fe_write() to
> qemu_chr_fe_write_all() to be complete and let the chardev handle
> EAGAIN.

No, using qemu_chr_fe_write_all() is even more wrong, because hw/char
devices should never block (and hw/char/serial.c handles flow control
properly).

However, indeed it seems to me that the logic of the patch is backwards:

- -1/EAGAIN should retry

- 0 should *not* retry, because it means the other side has hung up

- everything else should not retry.

Paolo

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Peter Maydell]

On 9 July 2018 at 14:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 09/07/2018 14:23, Marc-André Lureau wrote:
>> I suppose the patch should also change qemu_chr_fe_write() to
>> qemu_chr_fe_write_all() to be complete and let the chardev handle
>> EAGAIN.
>
> No, using qemu_chr_fe_write_all() is even more wrong, because hw/char
> devices should never block (and hw/char/serial.c handles flow control
> properly).
>
> However, indeed it seems to me that the logic of the patch is backwards:
>
> - -1/EAGAIN should retry
>
> - 0 should *not* retry, because it means the other side has hung up

This seems weird, because it doesn't follow the usual pattern
for non-blocking functions, where 0 just means "nothing was
written" and actual error conditions like the other side having
gone away are reported via -1 and some errno.

> - everything else should not retry.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Paolo Bonzini]

On 09/07/2018 15:21, Peter Maydell wrote:
>>
>> However, indeed it seems to me that the logic of the patch is backwards:
>>
>> - -1/EAGAIN should retry
>>
>> - 0 should *not* retry, because it means the other side has hung up
> This seems weird, because it doesn't follow the usual pattern
> for non-blocking functions, where 0 just means "nothing was
> written" and actual error conditions like the other side having
> gone away are reported via -1 and some errno.
> 

Ah no, that's read(2).  write(2) is as you say, so the patch should
check for EAGAIN.

Paolo

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Igor Mammedov]

On Tue, 5 Jun 2018 11:18:35 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/06/2018 09:54, Sergio Lopez wrote:
> > Only retry on serial_xmit if qemu_chr_fe_write returns 0, as this is the
> > only recoverable error.
> > 
> > Retrying with any other scenario, in addition to being a waste of CPU
> > cycles, can compromise the Guest stability if by the vCPU issuing the
> > write and the main loop thread are, by chance or explicit pinning,
> > running on the same pCPU.
> > 
> > Previous discussion:
> > 
> > https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06998.html
> > 
> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> > ---
> >  hw/char/serial.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 605b0d0..6de6c29 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -260,7 +260,7 @@ static void serial_xmit(SerialState *s)
> >          if (s->mcr & UART_MCR_LOOP) {
> >              /* in loopback mode, say that we just received a char */
> >              serial_receive1(s, &s->tsr, 1);
> > -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) != 1 &&
> > +        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
> >                     s->tsr_retry < MAX_XMIT_RETRY) {
> >              assert(s->watch_tag == 0);
> >              s->watch_tag =
> >   
Hi Sergio, Paolo,

it looks like commit 
commit 019288bf137183bf3407c9824655b753bfafc99f
Author: Sergio Lopez <slp@redhat.com>
Date:   Tue Jun 5 03:54:55 2018 -0400

    hw/char/serial: Only retry if qemu_chr_fe_write returns 0

introduced regression wrt 2.12 and broke windows guest remote kernel debug over
serial, where windbg can't connect to target and target hangs when windbg tries
to connect to it.

Reverting the commit fixes issue

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Marc-André Lureau]

Hi

On Tue, Jul 10, 2018 at 3:48 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 5 Jun 2018 11:18:35 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> On 05/06/2018 09:54, Sergio Lopez wrote:
>> > Only retry on serial_xmit if qemu_chr_fe_write returns 0, as this is the
>> > only recoverable error.
>> >
>> > Retrying with any other scenario, in addition to being a waste of CPU
>> > cycles, can compromise the Guest stability if by the vCPU issuing the
>> > write and the main loop thread are, by chance or explicit pinning,
>> > running on the same pCPU.
>> >
>> > Previous discussion:
>> >
>> > https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06998.html
>> >
>> > Signed-off-by: Sergio Lopez <slp@redhat.com>
>> > ---
>> >  hw/char/serial.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/hw/char/serial.c b/hw/char/serial.c
>> > index 605b0d0..6de6c29 100644
>> > --- a/hw/char/serial.c
>> > +++ b/hw/char/serial.c
>> > @@ -260,7 +260,7 @@ static void serial_xmit(SerialState *s)
>> >          if (s->mcr & UART_MCR_LOOP) {
>> >              /* in loopback mode, say that we just received a char */
>> >              serial_receive1(s, &s->tsr, 1);
>> > -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) != 1 &&
>> > +        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
>> >                     s->tsr_retry < MAX_XMIT_RETRY) {
>> >              assert(s->watch_tag == 0);
>> >              s->watch_tag =
>> >
> Hi Sergio, Paolo,
>
> it looks like commit
> commit 019288bf137183bf3407c9824655b753bfafc99f
> Author: Sergio Lopez <slp@redhat.com>
> Date:   Tue Jun 5 03:54:55 2018 -0400
>
>     hw/char/serial: Only retry if qemu_chr_fe_write returns 0
>
> introduced regression wrt 2.12 and broke windows guest remote kernel debug over
> serial, where windbg can't connect to target and target hangs when windbg tries
> to connect to it.
>
> Reverting the commit fixes issue
>

is this a possible solution?

diff --git a/hw/char/serial.c b/hw/char/serial.c
index cd7d747c68..046c4684ff 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -261,15 +261,20 @@ static void serial_xmit(SerialState *s)
         if (s->mcr & UART_MCR_LOOP) {
             /* in loopback mode, say that we just received a char */
             serial_receive1(s, &s->tsr, 1);
-        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
-                   s->tsr_retry < MAX_XMIT_RETRY) {
-            assert(s->watch_tag == 0);
-            s->watch_tag =
-                qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
-                                      serial_watch_cb, s);
-            if (s->watch_tag > 0) {
-                s->tsr_retry++;
-                return;
+        } else {
+            int rc = qemu_chr_fe_write(&s->chr, &s->tsr, 1);
+
+            if ((rc == 0 ||
+                 (rc == -1 && (errno == EAGAIN || errno == EINTR))) &&
+                s->tsr_retry < MAX_XMIT_RETRY) {
+                assert(s->watch_tag == 0);
+                s->watch_tag =
+                    qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                          serial_watch_cb, s);
+                if (s->watch_tag > 0) {
+                    s->tsr_retry++;
+                    return;
+                }
             }
         }
         s->tsr_retry = 0;


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Paolo Bonzini]

On 13/07/2018 16:55, Marc-André Lureau wrote:
> -                return;
> +        } else {
> +            int rc = qemu_chr_fe_write(&s->chr, &s->tsr, 1);
> +
> +            if ((rc == 0 ||
> +                 (rc == -1 && (errno == EAGAIN || errno == EINTR))) &&
> +                s->tsr_retry < MAX_XMIT_RETRY) {
> +                assert(s->watch_tag == 0);
> +                s->watch_tag =
> +                    qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                          serial_watch_cb, s);
> +                if (s->watch_tag > 0) {
> +                    s->tsr_retry++;
> +                    return;
> +                }

I think EINTR should be handled by chardev, otherwise yes.

Paolo

Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Igor Mammedov]

On Fri, 13 Jul 2018 16:55:15 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Tue, Jul 10, 2018 at 3:48 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 5 Jun 2018 11:18:35 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >  
> >> On 05/06/2018 09:54, Sergio Lopez wrote:  
> >> > Only retry on serial_xmit if qemu_chr_fe_write returns 0, as this is the
> >> > only recoverable error.
> >> >
> >> > Retrying with any other scenario, in addition to being a waste of CPU
> >> > cycles, can compromise the Guest stability if by the vCPU issuing the
> >> > write and the main loop thread are, by chance or explicit pinning,
> >> > running on the same pCPU.
> >> >
> >> > Previous discussion:
> >> >
> >> > https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06998.html
> >> >
> >> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> >> > ---
> >> >  hw/char/serial.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> >> > index 605b0d0..6de6c29 100644
> >> > --- a/hw/char/serial.c
> >> > +++ b/hw/char/serial.c
> >> > @@ -260,7 +260,7 @@ static void serial_xmit(SerialState *s)
> >> >          if (s->mcr & UART_MCR_LOOP) {
> >> >              /* in loopback mode, say that we just received a char */
> >> >              serial_receive1(s, &s->tsr, 1);
> >> > -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) != 1 &&
> >> > +        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
> >> >                     s->tsr_retry < MAX_XMIT_RETRY) {
> >> >              assert(s->watch_tag == 0);
> >> >              s->watch_tag =
> >> >  
> > Hi Sergio, Paolo,
> >
> > it looks like commit
> > commit 019288bf137183bf3407c9824655b753bfafc99f
> > Author: Sergio Lopez <slp@redhat.com>
> > Date:   Tue Jun 5 03:54:55 2018 -0400
> >
> >     hw/char/serial: Only retry if qemu_chr_fe_write returns 0
> >
> > introduced regression wrt 2.12 and broke windows guest remote kernel debug over
> > serial, where windbg can't connect to target and target hangs when windbg tries
> > to connect to it.
> >
> > Reverting the commit fixes issue
> >  
> 
> is this a possible solution?
it fixes windbg over serial (with and without EINTR)


> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index cd7d747c68..046c4684ff 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -261,15 +261,20 @@ static void serial_xmit(SerialState *s)
>          if (s->mcr & UART_MCR_LOOP) {
>              /* in loopback mode, say that we just received a char */
>              serial_receive1(s, &s->tsr, 1);
> -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
> -                   s->tsr_retry < MAX_XMIT_RETRY) {
> -            assert(s->watch_tag == 0);
> -            s->watch_tag =
> -                qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> -                                      serial_watch_cb, s);
> -            if (s->watch_tag > 0) {
> -                s->tsr_retry++;
> -                return;
> +        } else {
> +            int rc = qemu_chr_fe_write(&s->chr, &s->tsr, 1);
> +
> +            if ((rc == 0 ||
> +                 (rc == -1 && (errno == EAGAIN || errno == EINTR))) &&
> +                s->tsr_retry < MAX_XMIT_RETRY) {
> +                assert(s->watch_tag == 0);
> +                s->watch_tag =
> +                    qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                          serial_watch_cb, s);
> +                if (s->watch_tag > 0) {
> +                    s->tsr_retry++;
> +                    return;
> +                }
>              }
>          }
>          s->tsr_retry = 0;
> 
>