Avoid retrying on serial_xmit with EPIPE

QIO_CHANNEL_ERR_BROKEN is used to identify a potentially unrecoverable error in the channel, like EPIPE.

---
 include/io/channel.h | 1 +
 io/channel-file.c    | 3 +++
 2 files changed, 4 insertions(+)

include/io/channel.h +1 -0

index e8cdadb..bbe45f6 100644
@@ -38,6 +38,7 @@ typedef struct QIOChannel QIOChannel;
 typedef struct QIOChannelClass QIOChannelClass;
 
 #define QIO_CHANNEL_ERR_BLOCK -2
+#define QIO_CHANNEL_ERR_BROKEN -3
 
 typedef enum QIOChannelFeature QIOChannelFeature;
 

io/channel-file.c +0 -0

index db948ab..a990f67 100644

This allows callers to identify this potentially unrecoverable error.

---
 chardev/char-io.c | 3 +++
 1 file changed, 3 insertions(+)

chardev/char-io.c +3 -1

index f810524..f934eb9 100644
@@ -168,6 +168,9 @@ int io_channel_send_full(QIOChannel *ioc,
 
             errno = EAGAIN;
             return -1;
+        } else if (ret == QIO_CHANNEL_ERR_BROKEN) {
+            errno = EPIPE;
+            return -1;
         } else if (ret < 0) {
             errno = EINVAL;
             return -1;
--

1.8.3.1

Quoted 1 line · Sergio Lopez

This allows callers to identify this potentially unrecoverable error.

(missing signed-off)

This allows callers to identify this potentially unrecoverable error.

---
 chardev/char-io.c | 3 +++
 1 file changed, 3 insertions(+)

chardev/char-io.c +0 -0

index f810524..f934eb9 100644

If writing to the frontend channel failed with EPIPE, don't set up a retry. EPIPE is not a recoverable error, so trying again is a waste of CPU cycles.

If the vCPU writing to the serial device and emulator thread are pinned to the same pCPU, it can also compromise the stability of the Guest OS, as both threads will be competing for pCPU's time, with the vCPU actively polling the serial device and barely giving time to the emulator thread to make actual progress.

---
 hw/char/serial.c | 1 +
 1 file changed, 1 insertion(+)

hw/char/serial.c +0 -0

index 2c080c9..f26e86b 100644

hw/char/serial.c +0 -1

             assert(s->watch_tag == 0);
             s->watch_tag =
--

1.8.3.1

Quoted 13 lines · Sergio Lopez

If writing to the frontend channel failed with EPIPE, don't set up a retry. EPIPE is not a recoverable error, so trying again is a waste of CPU cycles.

If the vCPU writing to the serial device and emulator thread are pinned to the same pCPU, it can also compromise the stability of the Guest OS, as both threads will be competing for pCPU's time, with the vCPU actively polling the serial device and barely giving time to the emulator thread to make actual progress.

---
 hw/char/serial.c | 1 +
 1 file changed, 1 insertion(+)

hw/char/serial.c static void serial_xmit(SerialState *s) · line 266

Quoted 10 lines

hw/char/serial.c +1 -0

index 2c080c9..f26e86b 100644
@@ -262,6 +262,7 @@ static void serial_xmit(SerialState *s)
             /* 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 &&
+                   errno != EPIPE &&
                    s->tsr_retry < MAX_XMIT_RETRY) {

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

Quoted 14 lines · Marc-André Lureau

Quoted 13 lines · Sergio Lopez

If writing to the frontend channel failed with EPIPE, don't set up a retry. EPIPE is not a recoverable error, so trying again is a waste of CPU cycles.

If the vCPU writing to the serial device and emulator thread are pinned to the same pCPU, it can also compromise the stability of the Guest OS, as both threads will be competing for pCPU's time, with the vCPU actively polling the serial device and barely giving time to the emulator thread to make actual progress.

---
 hw/char/serial.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 13 lines

hw/char/serial.c static void serial_xmit(SerialState *s) · line 266

Quoted 10 lines

hw/char/serial.c +1 -0

index 2c080c9..f26e86b 100644
@@ -262,6 +262,7 @@ static void serial_xmit(SerialState *s)
             /* 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 &&
+                   errno != EPIPE &&
                    s->tsr_retry < MAX_XMIT_RETRY) {

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

What kind of action should it trigger if treating errno != EAGAIN as fatal?

If you meant something like calling 'abort()', I think that, while this could be considered as the "correct" behavior, it's a bit unpractical for the users. The most common situation for this to happen is restarting virtlogd while having the console/serial redirected to it. The VM will stop writing to the serial device, but otherwise the Guest OS can probably keep working without any issues.

Sergio.

Hi

Quoted 13 lines · Sergio Lopez

If writing to the frontend channel failed with EPIPE, don't set up a retry. EPIPE is not a recoverable error, so trying again is a waste of CPU cycles.

If the vCPU writing to the serial device and emulator thread are pinned to the same pCPU, it can also compromise the stability of the Guest OS, as both threads will be competing for pCPU's time, with the vCPU actively polling the serial device and barely giving time to the emulator thread to make actual progress.

---
 hw/char/serial.c | 1 +
 1 file changed, 1 insertion(+)

hw/char/serial.c static void serial_xmit(SerialState *s) · line 266

Quoted 10 lines

hw/char/serial.c +1 -0

index 2c080c9..f26e86b 100644
@@ -262,6 +262,7 @@ static void serial_xmit(SerialState *s)
             /* 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 &&
+                   errno != EPIPE &&
                    s->tsr_retry < MAX_XMIT_RETRY) {

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

hw/char/serial.c static void serial_xmit(SerialState *s) · line 268

Quoted 3 lines

hw/char/serial.c +0 -0

             assert(s->watch_tag == 0);
             s->watch_tag =

Quoted 16 lines · Marc-André Lureau

Hi

Quoted 13 lines · Sergio Lopez

If writing to the frontend channel failed with EPIPE, don't set up a retry. EPIPE is not a recoverable error, so trying again is a waste of CPU cycles.

If the vCPU writing to the serial device and emulator thread are pinned to the same pCPU, it can also compromise the stability of the Guest OS, as both threads will be competing for pCPU's time, with the vCPU actively polling the serial device and barely giving time to the emulator thread to make actual progress.

---
 hw/char/serial.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 13 lines

hw/char/serial.c static void serial_xmit(SerialState *s) · line 266

Quoted 10 lines

hw/char/serial.c +1 -0

index 2c080c9..f26e86b 100644
@@ -262,6 +262,7 @@ static void serial_xmit(SerialState *s)
             /* 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 &&
+                   errno != EPIPE &&
                    s->tsr_retry < MAX_XMIT_RETRY) {

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

Yes, exactly this code is already broken for every single errno value, not simply EPIPE. It needs fixing to treat '-1' return code correctly instead of retrying everything.

Quoted 5 lines

Quoted 2 lines

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

Yes, exactly this code is already broken for every single errno value, not simply EPIPE. It needs fixing to treat '-1' return code correctly instead of retrying everything.

Given that EAGAIN is already taken care of in chardev/char.c:qemu_chr_write_buffer, in which cases should we retry? Or should we just drop all the tsr_retry logic?

Quoted 5 lines · Sergio Lopez

Quoted 5 lines

Quoted 2 lines

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

Yes, exactly this code is already broken for every single errno value, not simply EPIPE. It needs fixing to treat '-1' return code correctly instead of retrying everything.

Quoted 3 lines

Given that EAGAIN is already taken care of in chardev/char.c:qemu_chr_write_buffer, in which cases should we retry? Or should we just drop all the tsr_retry logic?

It's not handled there, the call from qemu_chr_fe_write has write_all == false.

Quoted 10 lines · Paolo Bonzini

Quoted 8 lines · Sergio Lopez

Quoted 5 lines

Quoted 2 lines

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

Yes, exactly this code is already broken for every single errno value, not simply EPIPE. It needs fixing to treat '-1' return code correctly instead of retrying everything.

Given that EAGAIN is already taken care of in chardev/char.c:qemu_chr_write_buffer, in which cases should we retry? Or should we just drop all the tsr_retry logic?

Quoted 2 lines

It's not handled there, the call from qemu_chr_fe_write has write_all == false.

You're right. So should we just retry only for -1 && errno == EAGAIN and just ignore the error otherwise?

Quoted 10 lines · Paolo Bonzini

Quoted 8 lines · Sergio Lopez

Quoted 5 lines

Quoted 2 lines

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

Yes, exactly this code is already broken for every single errno value, not simply EPIPE. It needs fixing to treat '-1' return code correctly instead of retrying everything.

Given that EAGAIN is already taken care of in chardev/char.c:qemu_chr_write_buffer, in which cases should we retry? Or should we just drop all the tsr_retry logic?

Quoted 2 lines

It's not handled there, the call from qemu_chr_fe_write has write_all == false.

You're right. So should we just retry only for -1 && errno == EAGAIN and just ignore the error otherwise?

Quoted 11 lines · Sergio Lopez

Quoted 10 lines · Paolo Bonzini

Quoted 8 lines · Sergio Lopez

Quoted 5 lines

Quoted 2 lines

Instead of adding explicit handling of EPIPE, shouldn't the code be rewritten to treat -1 return && errno != EAGAIN as fatal?

Yes, exactly this code is already broken for every single errno value, not simply EPIPE. It needs fixing to treat '-1' return code correctly instead of retrying everything.

Given that EAGAIN is already taken care of in chardev/char.c:qemu_chr_write_buffer, in which cases should we retry? Or should we just drop all the tsr_retry logic?

Quoted 5 lines

Quoted 2 lines

It's not handled there, the call from qemu_chr_fe_write has write_all == false.

You're right. So should we just retry only for -1 && errno == EAGAIN and just ignore the error otherwise?

I don't think we should make all of qemu_chr_fe_write()'s callers have to handle EAGAIN. That function already has a way to tell the caller that it didn't consume any data, which is to return 0.

(EAGAIN is a curse and we should strive to avoid it spreading its poison into areas of the codebase that we can keep it out of.)

In general there are three cases I think qemu_chr_fe_write() callers might care about:
* data was consumed (return >0)
* data wasn't consumed, try again later (return 0)
* data wasn't consumed, and a later attempt won't work either (return -1)

NB that hw/char/serial.c is not the only serial device using this pattern and probably needing adjustment (though I think it's the only one with the complicated tsr_retry logic).

In the patch that started this thread the report is that we use lots of CPU. Can you explain why that happens? I was expecting that we would set up the qemu_chr_fe_add_watch() on the dead chr FE and it would just never fire since the FE can never accept further data...