PATCH: virtio_console: Fix poll blocking even though there is data to read

PATCH: virtio_console: Fix poll blocking even though there is data to read

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

Hi All,

I found this while working on a Linux agent for spice, the symptom I was
seeing was select blocking on the spice vdagent virtio serial port even
though there were messages queued up there.

I found this while working on a Linux agent for spice, the symptom I was
seeing was select blocking on the spice vdagent virtio serial port even
though there were messages queued up there.

virtio_console's port_fops_poll checks port->inbuf != NULL to determine if
read won't block. However if an application reads enough bytes from inbuf
through port_fops_read, to empty the current port->inbuf, port->inbuf
will be NULL even though there may be buffers left in the virtqueue.

This causes poll() to block even though there is data to be read, this patch
fixes this by using the alredy defined will_read_block utility function
instead of the port->inbuf != NULL check.

Signed-off-By: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

[-- Attachment #2: linux-2.6.35.4-virtio_console-fix-poll.patch --]
[-- Type: text/plain, Size: 1380 bytes --]

Subject: virtio_console: Fix poll blocking even though there is data to read
From: Hans de Goede <hdegoede@redhat.com>

I found this while working on a Linux agent for spice, the symptom I was
seeing was select blocking on the spice vdagent virtio serial port even
though there were messages queued up there.

virtio_console's port_fops_poll checks port->inbuf != NULL to determine if
read won't block. However if an application reads enough bytes from inbuf
through port_fops_read, to empty the current port->inbuf, port->inbuf
will be NULL even though there may be buffers left in the virtqueue.

This causes poll() to block even though there is data to be read, this patch
fixes this by using the alredy defined will_read_block utility function
instead of the port->inbuf != NULL check.

Signed-off-By: Hans de Goede <hdegoede@redhat.com>
diff -up linux-2.6.35.x86_64/drivers/char/virtio_console.c~ linux-2.6.35.x86_64/drivers/char/virtio_console.c
--- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
+++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
@@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
 	poll_wait(filp, &port->waitqueue, wait);
 
 	ret = 0;
-	if (port->inbuf)
+	if (!will_read_block(port))
 		ret |= POLLIN | POLLRDNORM;
 	if (!will_write_block(port))
 		ret |= POLLOUT;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Amit Shah]

On (Wed) Sep 15 2010 [15:04:53], Hans de Goede wrote:
> Hi All,
> 
> I found this while working on a Linux agent for spice, the symptom I was
> seeing was select blocking on the spice vdagent virtio serial port even
> though there were messages queued up there.
> 
> I found this while working on a Linux agent for spice, the symptom I was
> seeing was select blocking on the spice vdagent virtio serial port even
> though there were messages queued up there.
> 
> virtio_console's port_fops_poll checks port->inbuf != NULL to determine if
> read won't block. However if an application reads enough bytes from inbuf
> through port_fops_read, to empty the current port->inbuf, port->inbuf
> will be NULL even though there may be buffers left in the virtqueue.
> 
> This causes poll() to block even though there is data to be read, this patch
> fixes this by using the alredy defined will_read_block utility function
> instead of the port->inbuf != NULL check.
> 
> Signed-off-By: Hans de Goede <hdegoede@redhat.com>
> 
> Regards,
> 
> Hans

> diff -up linux-2.6.35.x86_64/drivers/char/virtio_console.c~ linux-2.6.35.x86_64/drivers/char/virtio_console.c
> --- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
> +++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
> @@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
>  	poll_wait(filp, &port->waitqueue, wait);
>  
>  	ret = 0;
> -	if (port->inbuf)
> +	if (!will_read_block(port))

Looks correct, but this should be

	if (port_has_data(port))

instead.

will_read_block() also tests if the host is connected, which is not what
we want for POLLIN to be set.

		Amit

Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Hans de Goede]

Hi,

On 09/15/2010 03:25 PM, Amit Shah wrote:
> On (Wed) Sep 15 2010 [15:04:53], Hans de Goede wrote:
>> Hi All,
>>
>> I found this while working on a Linux agent for spice, the symptom I was
>> seeing was select blocking on the spice vdagent virtio serial port even
>> though there were messages queued up there.
>>
>> I found this while working on a Linux agent for spice, the symptom I was
>> seeing was select blocking on the spice vdagent virtio serial port even
>> though there were messages queued up there.
>>
>> virtio_console's port_fops_poll checks port->inbuf != NULL to determine if
>> read won't block. However if an application reads enough bytes from inbuf
>> through port_fops_read, to empty the current port->inbuf, port->inbuf
>> will be NULL even though there may be buffers left in the virtqueue.
>>
>> This causes poll() to block even though there is data to be read, this patch
>> fixes this by using the alredy defined will_read_block utility function
>> instead of the port->inbuf != NULL check.
>>
>> Signed-off-By: Hans de Goede<hdegoede@redhat.com>
>>
>> Regards,
>>
>> Hans
>
>> diff -up linux-2.6.35.x86_64/drivers/char/virtio_console.c~ linux-2.6.35.x86_64/drivers/char/virtio_console.c
>> --- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
>> +++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
>> @@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
>>   	poll_wait(filp,&port->waitqueue, wait);
>>
>>   	ret = 0;
>> -	if (port->inbuf)
>> +	if (!will_read_block(port))
>
> Looks correct, but this should be
>
> 	if (port_has_data(port))
>
> instead.

That certainly works for me (as in will still fix the bug I'm hitting), but
quoting from "man 2 select":

        Three  independent  sets of file descriptors are watched.  Those listed
        in readfds will be watched to see if characters  become  available  for
        reading  (more  precisely, to see if a read will not block; in particu‐
        lar, a file descriptor is also ready on end-of-file)

Notice the "a file descriptor is also ready on end-of-file", and
port_fops_read treats the host not being connected as eof (it returns 0
in that case). So from an API pov I'm not sure what is correct.

Regards,

Hans
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Amit Shah]

On (Wed) Sep 15 2010 [15:37:21], Hans de Goede wrote:
> >>--- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
> >>+++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
> >>@@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
> >>  	poll_wait(filp,&port->waitqueue, wait);
> >>
> >>  	ret = 0;
> >>-	if (port->inbuf)
> >>+	if (!will_read_block(port))
> >
> >Looks correct, but this should be
> >
> >	if (port_has_data(port))
> >
> >instead.
> 
> That certainly works for me (as in will still fix the bug I'm hitting), but
> quoting from "man 2 select":
> 
>        Three  independent  sets of file descriptors are watched.  Those listed
>        in readfds will be watched to see if characters  become  available  for
>        reading  (more  precisely, to see if a read will not block; in particu‐
>        lar, a file descriptor is also ready on end-of-file)
> 
> Notice the "a file descriptor is also ready on end-of-file", and
> port_fops_read treats the host not being connected as eof (it returns 0
> in that case). So from an API pov I'm not sure what is correct.

poll(2) says:

              POLLIN There is data to read.

That makes it simple.

Also, we also set POLLHUP in case the host is disconnected, so if host
is not connected while there's data to read, we'll get POLLIN|POLLHUP in
revents, and further reads will be blocked.

I'm willing to go with the poll manpage and use port_has_data().

		Amit

Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Hans de Goede]

Hi,

On 09/15/2010 03:46 PM, Amit Shah wrote:
> On (Wed) Sep 15 2010 [15:37:21], Hans de Goede wrote:
>>>> --- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
>>>> +++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
>>>> @@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
>>>>   	poll_wait(filp,&port->waitqueue, wait);
>>>>
>>>>   	ret = 0;
>>>> -	if (port->inbuf)
>>>> +	if (!will_read_block(port))
>>>
>>> Looks correct, but this should be
>>>
>>> 	if (port_has_data(port))
>>>
>>> instead.
>>
>> That certainly works for me (as in will still fix the bug I'm hitting), but
>> quoting from "man 2 select":
>>
>>         Three  independent  sets of file descriptors are watched.  Those listed
>>         in readfds will be watched to see if characters  become  available  for
>>         reading  (more  precisely, to see if a read will not block; in particu‐
>>         lar, a file descriptor is also ready on end-of-file)
>>
>> Notice the "a file descriptor is also ready on end-of-file", and
>> port_fops_read treats the host not being connected as eof (it returns 0
>> in that case). So from an API pov I'm not sure what is correct.
>
> poll(2) says:
>
>                POLLIN There is data to read.
>
> That makes it simple.
>
> Also, we also set POLLHUP in case the host is disconnected, so if host
> is not connected while there's data to read, we'll get POLLIN|POLLHUP in
> revents, and further reads will be blocked.
>

Ah, I completely missed the POLLHUP getting set thingie. I guess that will
get used by the kernel to also not block if any fds have it said in select's
readfds argument.

One new patch coming up!

Regards,

Hans
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Rusty Russell]

On Wed, 15 Sep 2010 11:16:24 pm Amit Shah wrote:
> On (Wed) Sep 15 2010 [15:37:21], Hans de Goede wrote:
> > >>--- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
> > >>+++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
> > >>@@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
> > >>  	poll_wait(filp,&port->waitqueue, wait);
> > >>
> > >>  	ret = 0;
> > >>-	if (port->inbuf)
> > >>+	if (!will_read_block(port))
> > >
> > >Looks correct, but this should be
> > >
> > >	if (port_has_data(port))
> > >
> > >instead.
> > 
> > That certainly works for me (as in will still fix the bug I'm hitting), but
> > quoting from "man 2 select":
> > 
> >        Three  independent  sets of file descriptors are watched.  Those listed
> >        in readfds will be watched to see if characters  become  available  for
> >        reading  (more  precisely, to see if a read will not block; in particu‐
> >        lar, a file descriptor is also ready on end-of-file)
> > 
> > Notice the "a file descriptor is also ready on end-of-file", and
> > port_fops_read treats the host not being connected as eof (it returns 0
> > in that case). So from an API pov I'm not sure what is correct.
> 
> poll(2) says:
> 
>               POLLIN There is data to read.
> 
> That makes it simple.

That's a documentation bug.  On a pipe, POLLIN is set when the other end
closes.  read() then returns 0 immediately.

poll() sets POLLIN when read() won't block, and people count on it.

Let's do that, please.
Rusty.

Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Amit Shah]

On (Thu) Sep 16 2010 [15:32:54], Rusty Russell wrote:
> On Wed, 15 Sep 2010 11:16:24 pm Amit Shah wrote:
> > On (Wed) Sep 15 2010 [15:37:21], Hans de Goede wrote:
> > > >>--- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
> > > >>+++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
> > > >>@@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
> > > >>  	poll_wait(filp,&port->waitqueue, wait);
> > > >>
> > > >>  	ret = 0;
> > > >>-	if (port->inbuf)
> > > >>+	if (!will_read_block(port))
> > > >
> > > >Looks correct, but this should be
> > > >
> > > >	if (port_has_data(port))
> > > >
> > > >instead.
> > > 
> > > That certainly works for me (as in will still fix the bug I'm hitting), but
> > > quoting from "man 2 select":
> > > 
> > >        Three  independent  sets of file descriptors are watched.  Those listed
> > >        in readfds will be watched to see if characters  become  available  for
> > >        reading  (more  precisely, to see if a read will not block; in particu‐
> > >        lar, a file descriptor is also ready on end-of-file)
> > > 
> > > Notice the "a file descriptor is also ready on end-of-file", and
> > > port_fops_read treats the host not being connected as eof (it returns 0
> > > in that case). So from an API pov I'm not sure what is correct.
> > 
> > poll(2) says:
> > 
> >               POLLIN There is data to read.
> > 
> > That makes it simple.
> 
> That's a documentation bug.  On a pipe, POLLIN is set when the other end
> closes.  read() then returns 0 immediately.

Currently we don't set POLLIN when host goes down.  I'll do a second
patch for that.

> poll() sets POLLIN when read() won't block, and people count on it.

Yes, that's the behaviour with Hans's new patch as well -- that's not
changing.

The will_read_block() function (and the comment on top of it) are
causing this confusion:


/* The condition that must be true for polling to end */
static bool will_read_block(struct port *port)
{
	if (!port->guest_connected) {
		/* Port got hot-unplugged. Let's exit. */
		return false;
	}
	return !port_has_data(port) && port->host_connected;
}

This function is only called to unblock a blocking read() call.  So the
comment there has to be changed to read 'waiting' to end instead of
'polling' to end.

read() does return 0 immediately when the other end is not connected
(and there's no data to read).

In effect, we need:
- Hans's patch
- a patch to set POLLIN when host goes down (in addition to POLLHUP and
  SIGIO)
- a patch to change the comment for will_read_block.

Thanks,

		Amit

Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Amit Shah]

On (Thu) Sep 16 2010 [11:52:01], Amit Shah wrote:
> On (Thu) Sep 16 2010 [15:32:54], Rusty Russell wrote:
> > On Wed, 15 Sep 2010 11:16:24 pm Amit Shah wrote:
> > > On (Wed) Sep 15 2010 [15:37:21], Hans de Goede wrote:
> > > > >>--- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
> > > > >>+++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
> > > > >>@@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
> > > > >>  	poll_wait(filp,&port->waitqueue, wait);
> > > > >>
> > > > >>  	ret = 0;
> > > > >>-	if (port->inbuf)
> > > > >>+	if (!will_read_block(port))
> > > > >
> > > > >Looks correct, but this should be
> > > > >
> > > > >	if (port_has_data(port))
> > > > >
> > > > >instead.
> > > > 
> > > > That certainly works for me (as in will still fix the bug I'm hitting), but
> > > > quoting from "man 2 select":
> > > > 
> > > >        Three  independent  sets of file descriptors are watched.  Those listed
> > > >        in readfds will be watched to see if characters  become  available  for
> > > >        reading  (more  precisely, to see if a read will not block; in particu‐
> > > >        lar, a file descriptor is also ready on end-of-file)
> > > > 
> > > > Notice the "a file descriptor is also ready on end-of-file", and
> > > > port_fops_read treats the host not being connected as eof (it returns 0
> > > > in that case). So from an API pov I'm not sure what is correct.
> > > 
> > > poll(2) says:
> > > 
> > >               POLLIN There is data to read.
> > > 
> > > That makes it simple.
> > 
> > That's a documentation bug.  On a pipe, POLLIN is set when the other end
> > closes.  read() then returns 0 immediately.
> 
> Currently we don't set POLLIN when host goes down.  I'll do a second
> patch for that.
> 
> > poll() sets POLLIN when read() won't block, and people count on it.
> 
> Yes, that's the behaviour with Hans's new patch as well -- that's not
> changing.
> 
> The will_read_block() function (and the comment on top of it) are
> causing this confusion:
> 
> 
> /* The condition that must be true for polling to end */
> static bool will_read_block(struct port *port)
> {
> 	if (!port->guest_connected) {
> 		/* Port got hot-unplugged. Let's exit. */
> 		return false;
> 	}
> 	return !port_has_data(port) && port->host_connected;
> }
> 
> This function is only called to unblock a blocking read() call.  So the
> comment there has to be changed to read 'waiting' to end instead of
> 'polling' to end.
> 
> read() does return 0 immediately when the other end is not connected
> (and there's no data to read).
> 
> In effect, we need:
> - Hans's patch
> - a patch to set POLLIN when host goes down (in addition to POLLHUP and
>   SIGIO)
> - a patch to change the comment for will_read_block.

Uh, sorry for blabbering.

will_read_block() was written to be used in poll(), and the comment says
as much.  I should've used it right from the start.  Hans's first patch
was correct, I'll pick that up and that addresses all the "issues" that
are present.

Sorry again!

		Amit
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel