[Qemu-devel] [RFC] virtio-console fails on unconnected pty

[Qemu-devel] [RFC] virtio-console fails on unconnected pty

[Christian Borntraeger]

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?

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -163,7 +163,9 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
                 abort();
             }
             if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
-                virtio_serial_throttle_port(port, true);
+                if (!info->is_console) {
+                    virtio_serial_throttle_port(port, true);
+                }
                 port->iov_idx = i;
                 if (ret > 0) {
                     port->iov_offset += ret;

Re: [Qemu-devel] [RFC] virtio-console fails on unconnected pty

[Amit Shah]

Hi Christian,

On (Thu) 15 Dec 2011 [13:44:41], Christian Borntraeger wrote:
> 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.

Unfortunately we don't yet have chardevs to signal to frontends that
they're writable again.  This leads to things staying blocked, and
guests spinning.  (At least Fedora has patches that mitigate this
situation, after upstream acceptance of the patches failed.)

> 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.

Hm, looks like pty open/close notifications might be broken if you
never attached a pty to a console but the guest sent out stuff to the
host anyway.

> 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 should be temporary, though.  Once chardevs can notify frontends
about the ability to consume more data, this patch should be reverted,
right?

> This experimental patch seems to help. Makes sense?

If it's a temporary thing, I don't mind.  Can you please re-submit
without the RFC tag and perhaps a comment below to make explicit why
we're doing this?

> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -163,7 +163,9 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
>                  abort();
>              }
>              if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
> -                virtio_serial_throttle_port(port, true);
> +                if (!info->is_console) {
> +                    virtio_serial_throttle_port(port, true);
> +                }
>                  port->iov_idx = i;
>                  if (ret > 0) {
>                      port->iov_offset += ret;
> 

		Amit

[Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>
    
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 <borntraeger@de.ibm.com>

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

Index: b/hw/virtio-serial-bus.c
===================================================================
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -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);
+		/*
+                 * 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.
+                 * We can revert this if
+                 * 1: chardevs can notify frondends
+                 * 2: the guest driver does not spin in these cases
+                 */
+                if (!info->is_console) {
+                    virtio_serial_throttle_port(port, true);
+                }
                 port->iov_idx = i;
                 if (ret > 0) {
                     port->iov_offset += ret;

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Amit Shah]

On (Thu) 29 Dec 2011 [13:47:43], Christian Borntraeger wrote:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
>     
> 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.

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

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Andreas Färber]

Am 29.12.2011 14:27, schrieb Amit Shah:
> On (Thu) 29 Dec 2011 [13:47:43], Christian Borntraeger wrote:
>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>     
>> 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.

"attachment", while you're at it.

Andreas

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Amit Shah]

On (Thu) 29 Dec 2011 [13:47:43], Christian Borntraeger wrote:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
>     
> 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 <borntraeger@de.ibm.com>
> 
> ---
>  hw/virtio-serial-bus.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> Index: b/hw/virtio-serial-bus.c
> ===================================================================
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -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?

> +		/*
> +                 * 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.
> +                 * We can revert this if
> +                 * 1: chardevs can notify frondends
> +                 * 2: the guest driver does not spin in these cases
> +                 */
> +                if (!info->is_console) {
> +                    virtio_serial_throttle_port(port, true);
> +                }
>                  port->iov_idx = i;
>                  if (ret > 0) {
>                      port->iov_offset += ret;
> 

		Amit

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Christian Borntraeger]

>> +++ b/hw/virtio-serial-bus.c
>> @@ -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?

Christian

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Christian Borntraeger]

>> 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?

And of course this while

static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
                                 VirtIODevice *vdev)
[...]
    while (!port->throttled) {
        unsigned int i;

will also prevent further processing, as virtio_serial_throttle_port will 
set port-throttled.

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Amit Shah]

On (Thu) 29 Dec 2011 [15:16:55], Christian Borntraeger wrote:
> >> +++ b/hw/virtio-serial-bus.c
> >> @@ -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.  I'm just unsure how this
patch works for you :)

		Amit

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Christian Borntraeger]

> 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);
                                                   |
                                                   |
                                                   V
void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle)
{
    if (!port) {
        return;
    }

    trace_virtio_serial_throttle_port(port->id, throttle);
    port->throttled = throttle;              <-------------------- WHOOOT!
    if (throttle) {
        return;
    }
    qemu_bh_schedule(port->bh);
}

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Amit Shah]

On (Thu) 29 Dec 2011 [15:32:14], Christian Borntraeger wrote:
> > 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?


		Amit

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Christian Borntraeger]

On 29/12/11 15:56, Amit Shah wrote:
> On (Thu) 29 Dec 2011 [15:32:14], Christian Borntraeger wrote:
>>> 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?

ret is 0, see below for the gdb output. send_all does indeed break out of the
loop in case of 0. 

(gdb) bt
#0  do_flush_queued_data (port=0x8130a910, vq=0x813091e0, vdev=0x81309060) at /home/cborntra/REPOS/qemu/hw/virtio-serial-bus.c:166
#1  0x000000008022641e in handle_output (vdev=0x81309060, vq=0x813091e0) at /home/cborntra/REPOS/qemu/hw/virtio-serial-bus.c:481
#2  0x000000008022ac34 in virtio_queue_notify_vq (vq=0x813091e0) at /home/cborntra/REPOS/qemu/hw/virtio.c:632
#3  0x000000008022acc2 in virtio_queue_notify (vdev=0x81309060, n=1) at /home/cborntra/REPOS/qemu/hw/virtio.c:638
#4  0x00000000801e7168 in s390_virtio_hypercall (env=0x812f8170, mem=2097954816, hypercall=0) at /home/cborntra/REPOS/qemu/hw/s390-virtio.c:86
#5  0x00000000801bf3d4 in handle_hypercall (env=0x812f8170, run=0x20000028000) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:270
#6  0x00000000801bf48a in handle_diag (env=0x812f8170, run=0x20000028000, ipb_code=1280) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:281
#7  0x00000000801bfa3e in handle_instruction (env=0x812f8170, run=0x20000028000) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:397
#8  0x00000000801bfb76 in handle_intercept (env=0x812f8170) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:420
#9  0x00000000801bfcd2 in kvm_arch_handle_exit (env=0x812f8170, run=0x20000028000) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:452
#10 0x00000000801bd774 in kvm_cpu_exec (env=0x812f8170) at /home/cborntra/REPOS/qemu/kvm-all.c:1021
#11 0x0000000080181470 in qemu_kvm_cpu_thread_fn (arg=0x812f8170) at /home/cborntra/REPOS/qemu/cpus.c:740
#12 0x000002000043240e in start_thread () from /lib64/libpthread.so.0
#13 0x000002000055469a in thread_start () from /lib64/libc.so.6
(gdb) print ret
$3 = 0

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Amit Shah]

On (Thu) 29 Dec 2011 [15:32:14], Christian Borntraeger wrote:
> > 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.

		Amit

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Alexander Graf]

On 29.12.2011, at 16:14, Amit Shah wrote:

> On (Thu) 29 Dec 2011 [15:32:14], Christian Borntraeger wrote:
>>> 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.

So what's the conclusion then? :)


Alex

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Christian Borntraeger]

On 02/01/12 16:34, Alexander Graf wrote:
>>>            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.
> 
> 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.

Christian

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Alexander Graf]

On 02.01.2012, at 16:41, Christian Borntraeger wrote:

> On 02/01/12 16:34, Alexander Graf wrote:
>>>>           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.
>> 
>> 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 :)


Alex

Re: [Qemu-devel] [PATCHv2] Fix virtio-console failure on unconnected pty

[Amit Shah]

On (Mon) 02 Jan 2012 [17:55:06], Alexander Graf wrote:
> 
> On 02.01.2012, at 16:41, Christian Borntraeger wrote:
> 
> > On 02/01/12 16:34, Alexander Graf wrote:
> >>>>           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.
> >> 
> >> 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 :)

Yep, patches go through some tests first before sending out the pull
request.


		Amit