[PATCH] fix reading from character devices

[PATCH] fix reading from character devices

[Jan Kiszka]

Hi Avi,

commit "kvm: qemu: consume all pending I/O in I/O loop"
(8ab8bb09f1115b9bf733f885cc92b6c63d83f420) broke reading data bursts
from serial devices (and maybe from other character devices as well) by
guests. Reason: serial devices do input flow control via fd_read_poll,
but qemu now ignores this fact by pushing all data into the virtual
device as soon as it is available.

Patch below is not really nice (just as the whole internal virtual I/O
interface at the moment, IMHO), but it re-enables the serial ports for
now.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

---
 qemu/vl.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: kvm-userspace/qemu/vl.c
===================================================================
--- kvm-userspace.orig/qemu/vl.c
+++ kvm-userspace/qemu/vl.c
@@ -7751,7 +7751,10 @@ void main_loop_wait(int timeout)
         for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
             if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
-                more = 1;
+                if (!ioh->fd_read_poll || ioh->fd_read_poll(ioh->opaque))
+                    more = 1;
+                else
+                    FD_CLR(ioh->fd, &rfds);
             }
             if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
                 ioh->fd_write(ioh->opaque);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix reading from character devices

[Avi Kivity]

Jan Kiszka wrote:
> Hi Avi,
>
> commit "kvm: qemu: consume all pending I/O in I/O loop"
> (8ab8bb09f1115b9bf733f885cc92b6c63d83f420) broke reading data bursts
> from serial devices (and maybe from other character devices as well) by
> guests. Reason: serial devices do input flow control via fd_read_poll,
> but qemu now ignores this fact by pushing all data into the virtual
> device as soon as it is available.
>
> Patch below is not really nice (just as the whole internal virtual I/O
> interface at the moment, IMHO), but it re-enables the serial ports for
> now.
>
>   

I'm worried that it will break Dor's hack that speeds up virtio.  Dor?

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix reading from character devices

[Dor Laor]

On Mon, 2008-01-21 at 12:13 +0200, Avi Kivity wrote:
> Jan Kiszka wrote:
> > Hi Avi,
> >
> > commit "kvm: qemu: consume all pending I/O in I/O loop"
> > (8ab8bb09f1115b9bf733f885cc92b6c63d83f420) broke reading data bursts
> > from serial devices (and maybe from other character devices as well) by
> > guests. Reason: serial devices do input flow control via fd_read_poll,
> > but qemu now ignores this fact by pushing all data into the virtual
> > device as soon as it is available.
> >
> > Patch below is not really nice (just as the whole internal virtual I/O
> > interface at the moment, IMHO), but it re-enables the serial ports for
> > now.
> >
> >   
> 
> I'm worried that it will break Dor's hack that speeds up virtio.  Dor?
> 

It should be fine. Tap device without my hack has fd_read_poll null and
I hacked it to have a handler that returns false if virtio is used and
true otherwise. 
So Jan's patch should set 'more' to 1 and it will be like before.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix reading from character devices

[Avi Kivity]

Dor Laor wrote:
> On Mon, 2008-01-21 at 12:13 +0200, Avi Kivity wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> Hi Avi,
>>>
>>> commit "kvm: qemu: consume all pending I/O in I/O loop"
>>> (8ab8bb09f1115b9bf733f885cc92b6c63d83f420) broke reading data bursts
>>> from serial devices (and maybe from other character devices as well) by
>>> guests. Reason: serial devices do input flow control via fd_read_poll,
>>> but qemu now ignores this fact by pushing all data into the virtual
>>> device as soon as it is available.
>>>
>>> Patch below is not really nice (just as the whole internal virtual I/O
>>> interface at the moment, IMHO), but it re-enables the serial ports for
>>> now.
>>>
>>>   
>>>       
>> I'm worried that it will break Dor's hack that speeds up virtio.  Dor?
>>
>>     
>
> It should be fine. Tap device without my hack has fd_read_poll null and
> I hacked it to have a handler that returns false if virtio is used and
> true otherwise. 
> So Jan's patch should set 'more' to 1 and it will be like before.
>
>   

But if virtio is used with this patch, it won't set 'more' to 1.  Will 
virtio handle it or will throughput drop to be related to whatever 
timers we have set up?

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix reading from character devices

[Dor Laor]

On Mon, 2008-01-21 at 12:50 +0200, Avi Kivity wrote:
> Dor Laor wrote:
> > On Mon, 2008-01-21 at 12:13 +0200, Avi Kivity wrote:
> >   
> >> Jan Kiszka wrote:
> >>     
> >>> Hi Avi,
> >>>
> >>> commit "kvm: qemu: consume all pending I/O in I/O loop"
> >>> (8ab8bb09f1115b9bf733f885cc92b6c63d83f420) broke reading data bursts
> >>> from serial devices (and maybe from other character devices as well) by
> >>> guests. Reason: serial devices do input flow control via fd_read_poll,
> >>> but qemu now ignores this fact by pushing all data into the virtual
> >>> device as soon as it is available.
> >>>
> >>> Patch below is not really nice (just as the whole internal virtual I/O
> >>> interface at the moment, IMHO), but it re-enables the serial ports for
> >>> now.
> >>>
> >>>   
> >>>       
> >> I'm worried that it will break Dor's hack that speeds up virtio.  Dor?
> >>
> >>     
> >
> > It should be fine. Tap device without my hack has fd_read_poll null and
> > I hacked it to have a handler that returns false if virtio is used and
> > true otherwise. 
> > So Jan's patch should set 'more' to 1 and it will be like before.
> >
> >   
> 
> But if virtio is used with this patch, it won't set 'more' to 1.  Will 
> virtio handle it or will throughput drop to be related to whatever 
> timers we have set up?
> 

Virtio+tap with the performance hack does it's own select on the related
fd. Virtio+user will remain as before.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix reading from character devices

[Jan Kiszka]

Avi Kivity wrote:
> Dor Laor wrote:
>> On Mon, 2008-01-21 at 12:13 +0200, Avi Kivity wrote:
>>  
>>> Jan Kiszka wrote:
>>>    
>>>> Hi Avi,
>>>>
>>>> commit "kvm: qemu: consume all pending I/O in I/O loop"
>>>> (8ab8bb09f1115b9bf733f885cc92b6c63d83f420) broke reading data bursts
>>>> from serial devices (and maybe from other character devices as well) by
>>>> guests. Reason: serial devices do input flow control via fd_read_poll,
>>>> but qemu now ignores this fact by pushing all data into the virtual
>>>> device as soon as it is available.
>>>>
>>>> Patch below is not really nice (just as the whole internal virtual I/O
>>>> interface at the moment, IMHO), but it re-enables the serial ports for
>>>> now.
>>>>
>>>>         
>>> I'm worried that it will break Dor's hack that speeds up virtio.  Dor?
>>>
>>>     
>>
>> It should be fine. Tap device without my hack has fd_read_poll null and
>> I hacked it to have a handler that returns false if virtio is used and
>> true otherwise. So Jan's patch should set 'more' to 1 and it will be
>> like before.
>>
>>   
> 
> But if virtio is used with this patch, it won't set 'more' to 1.  Will
> virtio handle it or will throughput drop to be related to whatever
> timers we have set up?

As long as virtio_net_can_receive returns 1, the loop will keep on
feeding also this device. And if it returns 0, I see no reason for
ignoring this signal. Anythings else would be, well, weird. However, the
current io-polling loop in git urgently needs fixing, or even a redesign.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix reading from character devices

[Jan Kiszka]

Dor Laor wrote:
> On Mon, 2008-01-21 at 12:50 +0200, Avi Kivity wrote:
>> Dor Laor wrote:
>>> On Mon, 2008-01-21 at 12:13 +0200, Avi Kivity wrote:
>>>   
>>>> Jan Kiszka wrote:
>>>>     
>>>>> Hi Avi,
>>>>>
>>>>> commit "kvm: qemu: consume all pending I/O in I/O loop"
>>>>> (8ab8bb09f1115b9bf733f885cc92b6c63d83f420) broke reading data bursts
>>>>> from serial devices (and maybe from other character devices as well) by
>>>>> guests. Reason: serial devices do input flow control via fd_read_poll,
>>>>> but qemu now ignores this fact by pushing all data into the virtual
>>>>> device as soon as it is available.
>>>>>
>>>>> Patch below is not really nice (just as the whole internal virtual I/O
>>>>> interface at the moment, IMHO), but it re-enables the serial ports for
>>>>> now.
>>>>>
>>>>>   
>>>>>       
>>>> I'm worried that it will break Dor's hack that speeds up virtio.  Dor?
>>>>
>>>>     
>>> It should be fine. Tap device without my hack has fd_read_poll null and
>>> I hacked it to have a handler that returns false if virtio is used and
>>> true otherwise. 
>>> So Jan's patch should set 'more' to 1 and it will be like before.
>>>
>>>   
>> But if virtio is used with this patch, it won't set 'more' to 1.  Will 
>> virtio handle it or will throughput drop to be related to whatever 
>> timers we have set up?
>>
> 
> Virtio+tap with the performance hack does it's own select on the related
> fd. Virtio+user will remain as before.

I think this got lost somehow. Find refreshed patch below. Avi, could
you apply it kvm-userland to fix the still existent regression?

Jan

---
 qemu/vl.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: b/qemu/vl.c
===================================================================
--- a/qemu/vl.c
+++ b/qemu/vl.c
@@ -7786,7 +7786,10 @@ void main_loop_wait(int timeout)
         for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
             if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
-                more = 1;
+                if (!ioh->fd_read_poll || ioh->fd_read_poll(ioh->opaque))
+                    more = 1;
+                else
+                    FD_CLR(ioh->fd, &rfds);
             }
             if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
                 ioh->fd_write(ioh->opaque);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix reading from character devices

[Avi Kivity]

Jan Kiszka wrote:
> I think this got lost somehow. Find refreshed patch below. Avi, could
> you apply it kvm-userland to fix the still existent regression?
>
>   

Applied, thanks.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/