[PATCH] virtio: console: Don't block entire guest if host doesn't read data

[PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Amit Shah]

If the host is slow in reading data or doesn't read data at all,
blocking write calls not only blocked the program that called write()
but the entire guest itself.

To overcome this, let's not block till the host signals it has given
back the virtio ring element we passed it.  Instead, send the buffer to
the host and return to userspace.  This operation then becomes similar
to how non-blocking writes work, so let's use the existing code for this
path as well.

This code change also ensures blocking write calls do get blocked if
there's not enough room in the virtio ring as well as they don't return
-EAGAIN to userspace.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: stable@kernel.org
---
 drivers/char/virtio_console.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c810481..0f69c5e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -459,9 +459,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 
 	/*
 	 * Wait till the host acknowledges it pushed out the data we
-	 * sent.  This is done for ports in blocking mode or for data
-	 * from the hvc_console; the tty operations are performed with
-	 * spinlocks held so we can't sleep here.
+	 * sent.  This is done for data from the hvc_console; the tty
+	 * operations are performed with spinlocks held so we can't
+	 * sleep here.  An alternative would be to copy the data to a
+	 * buffer and relax the spinning requirement.  The downside is
+	 * we need to kmalloc a GFP_ATOMIC buffer each time the
+	 * console driver writes something out.
 	 */
 	while (!virtqueue_get_buf(out_vq, &len))
 		cpu_relax();
@@ -626,6 +629,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 		goto free_buf;
 	}
 
+	/*
+	 * We now ask send_buf() to not spin for generic ports -- we
+	 * can re-use the same code path that non-blocking file
+	 * descriptors take for blocking file descriptors since the
+	 * wait is already done and we're certain the write will go
+	 * through to the host.
+	 */
+	nonblock = true;
 	ret = send_buf(port, buf, count, nonblock);
 
 	if (nonblock && ret > 0)
-- 
1.7.2.3

Re: [PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Hans de Goede]

Hi,

On 10/19/2010 07:45 AM, Amit Shah wrote:
> If the host is slow in reading data or doesn't read data at all,
> blocking write calls not only blocked the program that called write()
> but the entire guest itself.
>
> To overcome this, let's not block till the host signals it has given
> back the virtio ring element we passed it.  Instead, send the buffer to
> the host and return to userspace.  This operation then becomes similar
> to how non-blocking writes work, so let's use the existing code for this
> path as well.
>
> This code change also ensures blocking write calls do get blocked if
> there's not enough room in the virtio ring as well as they don't return
> -EAGAIN to userspace.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> CC: stable@kernel.org
> ---
>   drivers/char/virtio_console.c |   17 ++++++++++++++---
>   1 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index c810481..0f69c5e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -459,9 +459,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
>
>   	/*
>   	 * Wait till the host acknowledges it pushed out the data we
> -	 * sent.  This is done for ports in blocking mode or for data
> -	 * from the hvc_console; the tty operations are performed with
> -	 * spinlocks held so we can't sleep here.
> +	 * sent.  This is done for data from the hvc_console; the tty
> +	 * operations are performed with spinlocks held so we can't
> +	 * sleep here.  An alternative would be to copy the data to a
> +	 * buffer and relax the spinning requirement.  The downside is
> +	 * we need to kmalloc a GFP_ATOMIC buffer each time the
> +	 * console driver writes something out.
>   	 */
>   	while (!virtqueue_get_buf(out_vq,&len))
>   		cpu_relax();
> @@ -626,6 +629,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>   		goto free_buf;
>   	}
>
> +	/*
> +	 * We now ask send_buf() to not spin for generic ports -- we
> +	 * can re-use the same code path that non-blocking file
> +	 * descriptors take for blocking file descriptors since the
> +	 * wait is already done and we're certain the write will go
> +	 * through to the host.
> +	 */
> +	nonblock = true;
>   	ret = send_buf(port, buf, count, nonblock);
>
>   	if (nonblock&&  ret>  0)

1) Hmm, this changes the code to kfree the buffer, but only if the send_buf
    succeeded (which it always should given we did a will_block check first).

    I cannot help but notice that the data was not freed on a blocking fd
    before this patch, but is freed now. And I see nothing in send_buf to make
    it take ownership of the buffer / free it in the blocking case, and not take
    ownership in the blocking case. More over if anything I would expect send_buf
    to take ownership in the non blocking case (as the data is not directly
    consumed there), and not take owner ship in the blocking case, but the check
    is the reverse. Also why is the buffer not freed if the write failed, that
    makes no sense.

2) Assuming that things are changed so that send_buf does take ownership of the
    buffer in the nonblocking case, shouldn't the buffer then be allocated
    with GPF_ATOMIC ?

3) This patch will cause processes filling the virtqueue fast enough to block
    to never wake up again, due to a missing waitqueue wakeup, see:
    https://bugzilla.redhat.com/show_bug.cgi?id=643750

Regards,

Hans

Re: [PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Hans de Goede]

Hi,

Ok replying to my own reply, because I misread the code.

On 10/19/2010 08:55 AM, Hans de Goede wrote:
> Hi,
>
> On 10/19/2010 07:45 AM, Amit Shah wrote:
>> If the host is slow in reading data or doesn't read data at all,
>> blocking write calls not only blocked the program that called write()
>> but the entire guest itself.
>>
>> To overcome this, let's not block till the host signals it has given
>> back the virtio ring element we passed it. Instead, send the buffer to
>> the host and return to userspace. This operation then becomes similar
>> to how non-blocking writes work, so let's use the existing code for this
>> path as well.
>>
>> This code change also ensures blocking write calls do get blocked if
>> there's not enough room in the virtio ring as well as they don't return
>> -EAGAIN to userspace.
>>
>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>> CC: stable@kernel.org
>> ---
>> drivers/char/virtio_console.c | 17 ++++++++++++++---
>> 1 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>> index c810481..0f69c5e 100644
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -459,9 +459,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
>>
>> /*
>> * Wait till the host acknowledges it pushed out the data we
>> - * sent. This is done for ports in blocking mode or for data
>> - * from the hvc_console; the tty operations are performed with
>> - * spinlocks held so we can't sleep here.
>> + * sent. This is done for data from the hvc_console; the tty
>> + * operations are performed with spinlocks held so we can't
>> + * sleep here. An alternative would be to copy the data to a
>> + * buffer and relax the spinning requirement. The downside is
>> + * we need to kmalloc a GFP_ATOMIC buffer each time the
>> + * console driver writes something out.
>> */
>> while (!virtqueue_get_buf(out_vq,&len))
>> cpu_relax();
>> @@ -626,6 +629,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>> goto free_buf;
>> }
>>
>> + /*
>> + * We now ask send_buf() to not spin for generic ports -- we
>> + * can re-use the same code path that non-blocking file
>> + * descriptors take for blocking file descriptors since the
>> + * wait is already done and we're certain the write will go
>> + * through to the host.
>> + */
>> + nonblock = true;
>> ret = send_buf(port, buf, count, nonblock);
>>
>> if (nonblock&& ret> 0)
>
> 1) Hmm, this changes the code to kfree the buffer, but only if the send_buf
> succeeded (which it always should given we did a will_block check first).
>
> I cannot help but notice that the data was not freed on a blocking fd
> before this patch, but is freed now. And I see nothing in send_buf to make
> it take ownership of the buffer / free it in the blocking case, and not take
> ownership in the blocking case.

This part still stands.

 > More over if anything I would expect send_buf
> to take ownership in the non blocking case (as the data is not directly
> consumed there), and not take owner ship in the blocking case, but the check
> is the reverse. Also why is the buffer not freed if the write failed, that
> makes no sense.
>

This part is wrong the:

if (nonblock && ret> 0)

Check make it jump (goto) over the free, so it does make sense, but is coded
rather convolutedly.

> 2) Assuming that things are changed so that send_buf does take ownership of the
> buffer in the nonblocking case, shouldn't the buffer then be allocated
> with GPF_ATOMIC ?
>
> 3) This patch will cause processes filling the virtqueue fast enough to block
> to never wake up again, due to a missing waitqueue wakeup, see:
> https://bugzilla.redhat.com/show_bug.cgi?id=643750

These 2 parts still stand.

Regards,

Hans

Re: [PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Amit Shah]

On (Tue) Oct 19 2010 [08:55:16], Hans de Goede wrote:
> Hi,
> 
> On 10/19/2010 07:45 AM, Amit Shah wrote:
> >If the host is slow in reading data or doesn't read data at all,
> >blocking write calls not only blocked the program that called write()
> >but the entire guest itself.
> >
> >To overcome this, let's not block till the host signals it has given
> >back the virtio ring element we passed it.  Instead, send the buffer to
> >the host and return to userspace.  This operation then becomes similar
> >to how non-blocking writes work, so let's use the existing code for this
> >path as well.
> >
> >This code change also ensures blocking write calls do get blocked if
> >there's not enough room in the virtio ring as well as they don't return
> >-EAGAIN to userspace.
> >
> >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> >CC: stable@kernel.org
> >---
> >  drivers/char/virtio_console.c |   17 ++++++++++++++---
> >  1 files changed, 14 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> >index c810481..0f69c5e 100644
> >--- a/drivers/char/virtio_console.c
> >+++ b/drivers/char/virtio_console.c
> >@@ -459,9 +459,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
> >
> >  	/*
> >  	 * Wait till the host acknowledges it pushed out the data we
> >-	 * sent.  This is done for ports in blocking mode or for data
> >-	 * from the hvc_console; the tty operations are performed with
> >-	 * spinlocks held so we can't sleep here.
> >+	 * sent.  This is done for data from the hvc_console; the tty
> >+	 * operations are performed with spinlocks held so we can't
> >+	 * sleep here.  An alternative would be to copy the data to a
> >+	 * buffer and relax the spinning requirement.  The downside is
> >+	 * we need to kmalloc a GFP_ATOMIC buffer each time the
> >+	 * console driver writes something out.
> >  	 */
> >  	while (!virtqueue_get_buf(out_vq,&len))
> >  		cpu_relax();
> >@@ -626,6 +629,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> >  		goto free_buf;
> >  	}
> >
> >+	/*
> >+	 * We now ask send_buf() to not spin for generic ports -- we
> >+	 * can re-use the same code path that non-blocking file
> >+	 * descriptors take for blocking file descriptors since the
> >+	 * wait is already done and we're certain the write will go
> >+	 * through to the host.
> >+	 */
> >+	nonblock = true;
> >  	ret = send_buf(port, buf, count, nonblock);
> >
> >  	if (nonblock&&  ret>  0)
> 
> 1) Hmm, this changes the code to kfree the buffer, but only if the send_buf
>    succeeded (which it always should given we did a will_block check first).

The change is to *not* free the buffer.  It will be freed later when the
host indicates it's done with it (happens in reclaim_consumed_buffers()).

>    I cannot help but notice that the data was not freed on a blocking fd
>    before this patch, but is freed now. And I see nothing in send_buf to make
>    it take ownership of the buffer / free it in the blocking case, and not take
>    ownership in the blocking case. More over if anything I would expect send_buf
>    to take ownership in the non blocking case (as the data is not directly
>    consumed there), and not take owner ship in the blocking case, but the check
>    is the reverse. Also why is the buffer not freed if the write failed, that
>    makes no sense.

The buffer used to be freed in the blocking case, as we knew for certain
the host was done with the buffer.  Now it's not, we'll free it later.

> 2) Assuming that things are changed so that send_buf does take ownership of the
>    buffer in the nonblocking case, shouldn't the buffer then be allocated
>    with GPF_ATOMIC ?

Why?  We're not called from irq context.

> 3) This patch will cause processes filling the virtqueue fast enough to block
>    to never wake up again, due to a missing waitqueue wakeup, see:
>    https://bugzilla.redhat.com/show_bug.cgi?id=643750

Doesn't happen in my testcase, but this patch shouldn't cause that
problem if it exists -- it's a problem that exists even now for
nonblocking ports.  So if such a bug exists, it needs to be fixed
independently.

		Amit

Re: [PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Amit Shah]

On (Tue) Oct 19 2010 [08:57:43], Hans de Goede wrote:
> Hi,
> 
> Ok replying to my own reply, because I misread the code.
> 
> On 10/19/2010 08:55 AM, Hans de Goede wrote:
> >Hi,
> >
> >On 10/19/2010 07:45 AM, Amit Shah wrote:
> >>If the host is slow in reading data or doesn't read data at all,
> >>blocking write calls not only blocked the program that called write()
> >>but the entire guest itself.
> >>
> >>To overcome this, let's not block till the host signals it has given
> >>back the virtio ring element we passed it. Instead, send the buffer to
> >>the host and return to userspace. This operation then becomes similar
> >>to how non-blocking writes work, so let's use the existing code for this
> >>path as well.
> >>
> >>This code change also ensures blocking write calls do get blocked if
> >>there's not enough room in the virtio ring as well as they don't return
> >>-EAGAIN to userspace.
> >>
> >>Signed-off-by: Amit Shah<amit.shah@redhat.com>
> >>CC: stable@kernel.org
> >>---
> >>drivers/char/virtio_console.c | 17 ++++++++++++++---
> >>1 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> >>index c810481..0f69c5e 100644
> >>--- a/drivers/char/virtio_console.c
> >>+++ b/drivers/char/virtio_console.c
> >>@@ -459,9 +459,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
> >>
> >>/*
> >>* Wait till the host acknowledges it pushed out the data we
> >>- * sent. This is done for ports in blocking mode or for data
> >>- * from the hvc_console; the tty operations are performed with
> >>- * spinlocks held so we can't sleep here.
> >>+ * sent. This is done for data from the hvc_console; the tty
> >>+ * operations are performed with spinlocks held so we can't
> >>+ * sleep here. An alternative would be to copy the data to a
> >>+ * buffer and relax the spinning requirement. The downside is
> >>+ * we need to kmalloc a GFP_ATOMIC buffer each time the
> >>+ * console driver writes something out.
> >>*/
> >>while (!virtqueue_get_buf(out_vq,&len))
> >>cpu_relax();
> >>@@ -626,6 +629,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> >>goto free_buf;
> >>}
> >>
> >>+ /*
> >>+ * We now ask send_buf() to not spin for generic ports -- we
> >>+ * can re-use the same code path that non-blocking file
> >>+ * descriptors take for blocking file descriptors since the
> >>+ * wait is already done and we're certain the write will go
> >>+ * through to the host.
> >>+ */
> >>+ nonblock = true;
> >>ret = send_buf(port, buf, count, nonblock);
> >>
> >>if (nonblock&& ret> 0)
> >
> >1) Hmm, this changes the code to kfree the buffer, but only if the send_buf
> >succeeded (which it always should given we did a will_block check first).
> >
> >I cannot help but notice that the data was not freed on a blocking fd
> >before this patch, but is freed now. And I see nothing in send_buf to make
> >it take ownership of the buffer / free it in the blocking case, and not take
> >ownership in the blocking case.
> 
> This part still stands.
> 
> > More over if anything I would expect send_buf
> >to take ownership in the non blocking case (as the data is not directly
> >consumed there), and not take owner ship in the blocking case, but the check
> >is the reverse. Also why is the buffer not freed if the write failed, that
> >makes no sense.
> >
> 
> This part is wrong the:
> 
> if (nonblock && ret> 0)
>
> Check make it jump (goto) over the free, so it does make sense, but is coded
> rather convolutedly.

This means that:

a) we're not going to wait for the host to tell us it used the buffer,
so keep the buffer around.

b) we actually managed to send the buffer to the host.

I don't see why that's convoluted :-)

		Amit

Re: [PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Hans de Goede]

Hi,

On 10/19/2010 09:10 AM, Amit Shah wrote:
> On (Tue) Oct 19 2010 [08:55:16], Hans de Goede wrote:
>> Hi,
>>
>> On 10/19/2010 07:45 AM, Amit Shah wrote:
>>> If the host is slow in reading data or doesn't read data at all,
>>> blocking write calls not only blocked the program that called write()
>>> but the entire guest itself.
>>>
>>> To overcome this, let's not block till the host signals it has given
>>> back the virtio ring element we passed it.  Instead, send the buffer to
>>> the host and return to userspace.  This operation then becomes similar
>>> to how non-blocking writes work, so let's use the existing code for this
>>> path as well.
>>>
>>> This code change also ensures blocking write calls do get blocked if
>>> there's not enough room in the virtio ring as well as they don't return
>>> -EAGAIN to userspace.
>>>
>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>> CC: stable@kernel.org
>>> ---
>>>   drivers/char/virtio_console.c |   17 ++++++++++++++---
>>>   1 files changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>>> index c810481..0f69c5e 100644
>>> --- a/drivers/char/virtio_console.c
>>> +++ b/drivers/char/virtio_console.c
>>> @@ -459,9 +459,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
>>>
>>>   	/*
>>>   	 * Wait till the host acknowledges it pushed out the data we
>>> -	 * sent.  This is done for ports in blocking mode or for data
>>> -	 * from the hvc_console; the tty operations are performed with
>>> -	 * spinlocks held so we can't sleep here.
>>> +	 * sent.  This is done for data from the hvc_console; the tty
>>> +	 * operations are performed with spinlocks held so we can't
>>> +	 * sleep here.  An alternative would be to copy the data to a
>>> +	 * buffer and relax the spinning requirement.  The downside is
>>> +	 * we need to kmalloc a GFP_ATOMIC buffer each time the
>>> +	 * console driver writes something out.
>>>   	 */
>>>   	while (!virtqueue_get_buf(out_vq,&len))
>>>   		cpu_relax();
>>> @@ -626,6 +629,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>>>   		goto free_buf;
>>>   	}
>>>
>>> +	/*
>>> +	 * We now ask send_buf() to not spin for generic ports -- we
>>> +	 * can re-use the same code path that non-blocking file
>>> +	 * descriptors take for blocking file descriptors since the
>>> +	 * wait is already done and we're certain the write will go
>>> +	 * through to the host.
>>> +	 */
>>> +	nonblock = true;
>>>   	ret = send_buf(port, buf, count, nonblock);
>>>
>>>   	if (nonblock&&   ret>   0)
>>
>> 1) Hmm, this changes the code to kfree the buffer, but only if the send_buf
>>     succeeded (which it always should given we did a will_block check first).
>
> The change is to *not* free the buffer.  It will be freed later when the
> host indicates it's done with it (happens in reclaim_consumed_buffers()).
>

Ah, thanks for explaining that.

>>     I cannot help but notice that the data was not freed on a blocking fd
>>     before this patch, but is freed now. And I see nothing in send_buf to make
>>     it take ownership of the buffer / free it in the blocking case, and not take
>>     ownership in the blocking case. More over if anything I would expect send_buf
>>     to take ownership in the non blocking case (as the data is not directly
>>     consumed there), and not take owner ship in the blocking case, but the check
>>     is the reverse. Also why is the buffer not freed if the write failed, that
>>     makes no sense.
>
> The buffer used to be freed in the blocking case, as we knew for certain
> the host was done with the buffer.  Now it's not, we'll free it later.
>
>> 2) Assuming that things are changed so that send_buf does take ownership of the
>>     buffer in the nonblocking case, shouldn't the buffer then be allocated
>>     with GPF_ATOMIC ?
>
> Why?  We're not called from irq context.
>

Ok, my bad.

>> 3) This patch will cause processes filling the virtqueue fast enough to block
>>     to never wake up again, due to a missing waitqueue wakeup, see:
>>     https://bugzilla.redhat.com/show_bug.cgi?id=643750
>
> Doesn't happen in my testcase, but this patch shouldn't cause that
> problem if it exists -- it's a problem that exists even now for
> nonblocking ports.  So if such a bug exists, it needs to be fixed
> independently.

First of all lets agree that this is a real problem, there is simply nothing
waking the waitqueue were fops_write (or poll) block on when buffers become
available in out_vq, it may be hard to come up with a test case which fills
the queue fast enough to hit this scenario, but it is very real.

I agree it is an independent problem, and should be fixed in a separate
patch, but that patch should be part of the same set and become *before*
this one, as this patch now extends the problem to ports opened in blocking
mode too.

BTW, many thanks for working on this, it is appreciated :)

Regards,

Hans

Re: [PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Amit Shah]

On (Tue) Oct 19 2010 [09:23:00], Hans de Goede wrote:
> >>3) This patch will cause processes filling the virtqueue fast enough to block
> >>    to never wake up again, due to a missing waitqueue wakeup, see:
> >>    https://bugzilla.redhat.com/show_bug.cgi?id=643750
> >
> >Doesn't happen in my testcase, but this patch shouldn't cause that
> >problem if it exists -- it's a problem that exists even now for
> >nonblocking ports.  So if such a bug exists, it needs to be fixed
> >independently.
> 
> First of all lets agree that this is a real problem,

Sure, got a testcase for the test-virtserial or kvm-autotest projects?
;-)

I did try it and POLLOUT gets set for me immediately when I read one
buffer from the host.

> there is simply nothing
> waking the waitqueue were fops_write (or poll) block on when buffers become
> available in out_vq, it may be hard to come up with a test case which fills
> the queue fast enough to hit this scenario, but it is very real.

Not at all.

Connect guest
Connect host

On guest, check for POLLOUT.  As long as it's set, write buffers.
When POLLOUT goes off, read one buffer from host.  See if POLLOUT is set
again.

Also, as I mentioned in a private chat, the fix for that problem is easy
enough.

> I agree it is an independent problem, and should be fixed in a separate
> patch, but that patch should be part of the same set and become *before*
> this one, as this patch now extends the problem to ports opened in blocking
> mode too.

Strongly disagree.  This patch fixes a problem wherein blocking-mode
writes to a port freeze the entire guest.  That's a much uglier problem
to have than poll not indicating a port is writable again.

> BTW, many thanks for working on this, it is appreciated :)

Sure, thanks :-)

		Amit

Re: [PATCH] virtio: console: Don't block entire guest if host doesn't read data

[Hans de Goede]

Hi,

On 10/19/2010 09:32 AM, Amit Shah wrote:
> On (Tue) Oct 19 2010 [09:23:00], Hans de Goede wrote:
>>>> 3) This patch will cause processes filling the virtqueue fast enough to block
>>>>     to never wake up again, due to a missing waitqueue wakeup, see:
>>>>     https://bugzilla.redhat.com/show_bug.cgi?id=643750
>>>
>>> Doesn't happen in my testcase, but this patch shouldn't cause that
>>> problem if it exists -- it's a problem that exists even now for
>>> nonblocking ports.  So if such a bug exists, it needs to be fixed
>>> independently.
>>
>> First of all lets agree that this is a real problem,
>
> Sure, got a testcase for the test-virtserial or kvm-autotest projects?
> ;-)
>
> I did try it and POLLOUT gets set for me immediately when I read one
> buffer from the host.
>

Ok, weird but ok :)

<snip>

>> I agree it is an independent problem, and should be fixed in a separate
>> patch, but that patch should be part of the same set and become *before*
>> this one, as this patch now extends the problem to ports opened in blocking
>> mode too.
>
> Strongly disagree.  This patch fixes a problem wherein blocking-mode
> writes to a port freeze the entire guest.  That's a much uglier problem
> to have than poll not indicating a port is writable again.

Ack, you're right.

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans