virtio-console fails on unconnected pty

Amit,

when I tried qemu with -virtio-console pty the guest hangs and attaching on /dev/pts/<x> does not return anything if the attachement is too late.

Turns out that the console is already throttled and the guest is heavily spinning but get_buf never returns the buffer. There seems to be no way for the console to unthrottle the port.

For the virtio-serial use case we dont want to loose data but for the console we better drop data instead of "killing" the guest console. The old serial console also drops data after a retry, so what about dropping data righ away if no listener is connected to the pty?

This experimental patch seems to help. Makes sense?

From: Christian Borntraeger <[email protected]>

when I tried qemu with -virtio-console pty the guest hangs and attaching on /dev/pts/<x> does not return anything if the attachement is too late.
Turns out that the console is already throttled and the guest is heavily spinning but get_buf never returns the buffer. There seems to be no way for the console to unthrottle the port.

Quoted 4 lines

From: Christian Borntraeger <[email protected]>

when I tried qemu with -virtio-console pty the guest hangs and attaching on /dev/pts/<x> does not return anything if the attachement is too late.

Quoted 2 lines

Turns out that the console is already throttled and the guest is heavily spinning but get_buf never returns the buffer. There seems to be no way for the console to unthrottle the port.

It doesn't really get throttled, since the throttling enabling/disabling isn't yet done in QEMU. What happens is that the chardev buffer gets filled up and qemu_chr_fe_write() doesn't return -- send_all() keeps spinning.

I'll adjust the commit log message and submit.

Thanks.

Amit

From: Christian Borntraeger <[email protected]>

when I tried qemu with -virtio-console pty the guest hangs and attaching on /dev/pts/<x> does not return anything if the attachement is too late.
Turns out that the console is already throttled and the guest is heavily spinning but get_buf never returns the buffer. There seems to be no way for the console to unthrottle the port.

For the virtio-serial use case we dont want to loose data but for the console case we better drop data instead of "killing" the guest console. If we get chardev->frontend notification and a better behaving virtio-console we can revert this fix.

Signed-off-by Christian Borntraeger <[email protected]>

---
 hw/virtio-serial-bus.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Index: b/hw/virtio-serial-bus.c
===================================================================

hw/virtio-serial-bus.c +6 -0

+		/*
+                 * this is a temporary check until chardevs can signal to
+                 * frontends that they are writable again. This prevents
+                 * the console from going into throttled mode (forever)
+                 * if virtio-console is connected to a pty without a
+                 * listener. Otherwise the guest spins forever.

hw/virtio-serial-bus.c static void do_flush_queued_data(VirtIOS · line 166

Quoted 6 lines

+++ b/hw/virtio-serial-bus.c

hw/virtio-serial-bus.c +0 -1

@@ -163,7 +163,19 @@ static void do_flush_queued_data(VirtIOS
                 abort();
             }
             if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
-                virtio_serial_throttle_port(port, true);

I'm surprised: did you test this with upstream qemu? That codebase doesn't yet throttle writes, and this code path won't execute. Does it really not reproduce with this patch?

Quoted 3 lines

I'm surprised: did you test this with upstream qemu? That codebase doesn't yet throttle writes, and this code path won't execute. Does it really not reproduce with this patch?

I think
static void handle_output(VirtIODevice *vdev, VirtQueue *vq) [....]

    if (!port->throttled) {
        do_flush_queued_data(port, vq, vdev);
        return;

makes a difference here, since we will never return the buffer to the guest, no?

Quoted 10 lines

hw/virtio-serial-bus.c static void do_flush_queued_data(VirtIOS · line 166

Quoted 6 lines

+++ b/hw/virtio-serial-bus.c

hw/virtio-serial-bus.c +0 -1

@@ -163,7 +163,19 @@ static void do_flush_queued_data(VirtIOS
                 abort();
             }
             if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
-                virtio_serial_throttle_port(port, true);

I'm surprised: did you test this with upstream qemu? That codebase doesn't yet throttle writes, and this code path won't execute. Does it really not reproduce with this patch?

I think
static void handle_output(VirtIODevice *vdev, VirtQueue *vq) [....]

    if (!port->throttled) {
        do_flush_queued_data(port, vq, vdev);
        return;

makes a difference here, since we will never return the buffer to the guest, no?

port->throttled never becomes true for qemu.

Quoted 1 line

port->throttled never becomes true for qemu.

Huh? What did I miss below?

            if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
                virtio_serial_throttle_port(port, true);

Quoted 6 lines · Christian Borntraeger

Quoted 1 line

port->throttled never becomes true for qemu.

Huh? What did I miss below?

            if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
                virtio_serial_throttle_port(port, true);

'ret' here is the return value from info->have_data(), which is hw/virtio-console.c:flush_buf(). That function returns the value that qemu_chr_fe_write() returns, which is qemu-char.c:send_all() for pty, tcp, unix sockets. send_all() just keeps spinning here if it can't write, doesn't signal EAGAIN at all (or even a partial write).

Can you print out the return values of have_data to check what's going on? Maybe you're hitting a case I never hit earlier and throttling indeed does get enabled?

Quoted 1 line

port->throttled never becomes true for qemu.

Huh? What did I miss below?

            if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
                virtio_serial_throttle_port(port, true);

Quoted 6 lines · Christian Borntraeger

Quoted 1 line

port->throttled never becomes true for qemu.

Huh? What did I miss below?

           if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
               virtio_serial_throttle_port(port, true);

Ah; I see what's happening. pty_chr_write() returns 0 if a client isn't yet connected.

This is different from the buggy tcp_chr_write() code, which just returns 'len', i.e. a successful write. And since I've only tested with tcp/unix sockets, I couldn't see why throttling could get enabled.

Quoted 6 lines

Quoted 2 lines

           if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
               virtio_serial_throttle_port(port, true);

Ah; I see what's happening. pty_chr_write() returns 0 if a client isn't yet connected.

Quoted 4 lines

This is different from the buggy tcp_chr_write() code, which just returns 'len', i.e. a successful write. And since I've only tested with tcp/unix sockets, I couldn't see why throttling could get enabled.

So what's the conclusion then? :)

Quoted 11 lines · Alexander Graf

Quoted 10 lines

Quoted 2 lines

          if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
              virtio_serial_throttle_port(port, true);

Ah; I see what's happening. pty_chr_write() returns 0 if a client isn't yet connected.

This is different from the buggy tcp_chr_write() code, which just returns 'len', i.e. a successful write. And since I've only tested with tcp/unix sockets, I couldn't see why throttling could get enabled.

Quoted 1 line

So what's the conclusion then? :)

My conclusion is that with my patch virtio-console on a pty works, without it doesnt. :-)
I think Amit said that he is going to apply the patch.

Quoted 12 lines · Christian Borntraeger

Quoted 11 lines · Alexander Graf

Quoted 10 lines

Quoted 2 lines

          if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
              virtio_serial_throttle_port(port, true);

Ah; I see what's happening. pty_chr_write() returns 0 if a client isn't yet connected.

This is different from the buggy tcp_chr_write() code, which just returns 'len', i.e. a successful write. And since I've only tested with tcp/unix sockets, I couldn't see why throttling could get enabled.

Quoted 5 lines

Quoted 1 line

So what's the conclusion then? :)

My conclusion is that with my patch virtio-console on a pty works, without it doesnt. :-)
I think Amit said that he is going to apply the patch.

Ah, alright then :)