[Qemu-devel] [4341] qemu: generate signals on tap I/O

[Qemu-devel] [4341] qemu: generate signals on tap I/O

[Aurelien Jarno]

Revision: 4341
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4341
Author:   aurel32
Date:     2008-05-05 21:26:43 +0000 (Mon, 05 May 2008)

Log Message:
-----------
qemu: generate signals on tap I/O

Currently tap does not generate signals on I/O; this causes
network latency to be dependent on the timer tick (1ms without
dyntick, guest dependent with dyntick).  By generating a signal
on I/O, we can inform the guest immediately that a packet has
arrived.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Modified Paths:
--------------
    trunk/vl.c

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2008-05-05 21:26:31 UTC (rev 4340)
+++ trunk/vl.c	2008-05-05 21:26:43 UTC (rev 4341)
@@ -4030,6 +4030,7 @@
     if (!s)
         return NULL;
     s->fd = fd;
+    enable_sigio_timer(fd);
     s->vc = qemu_new_vlan_client(vlan, tap_receive, NULL, s);
     qemu_set_fd_handler(s->fd, tap_send, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "tap: fd=%d", fd);

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anders]

Aurelien Jarno wrote:
> --- trunk/vl.c	2008-05-05 21:26:31 UTC (rev 4340)
> +++ trunk/vl.c	2008-05-05 21:26:43 UTC (rev 4341)
> @@ -4030,6 +4030,7 @@
>      if (!s)
>          return NULL;
>      s->fd = fd;
> +    enable_sigio_timer(fd);
>      s->vc = qemu_new_vlan_client(vlan, tap_receive, NULL, s);
>      qemu_set_fd_handler(s->fd, tap_send, NULL, s);
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str), "tap: fd=%d", fd)

Hm, I think this will break on BSD, as the function is only available in 
Linux?

I posted a more complete implementation a few weeks ago, anything wrong 
with it?

http://thread.gmane.org/gmane.comp.emulators.qemu/24567


Cheers,
Anders.

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Aurelien Jarno]

Anders a écrit :
> Aurelien Jarno wrote:
>> --- trunk/vl.c    2008-05-05 21:26:31 UTC (rev 4340)
>> +++ trunk/vl.c    2008-05-05 21:26:43 UTC (rev 4341)
>> @@ -4030,6 +4030,7 @@
>>      if (!s)
>>          return NULL;
>>      s->fd = fd;
>> +    enable_sigio_timer(fd);
>>      s->vc = qemu_new_vlan_client(vlan, tap_receive, NULL, s);
>>      qemu_set_fd_handler(s->fd, tap_send, NULL, s);
>>      snprintf(s->vc->info_str, sizeof(s->vc->info_str), "tap: fd=%d", fd)
> 
> Hm, I think this will break on BSD, as the function is only available in
> Linux?

That's why I reverted that commit, it was for my local tree only, I
committed it accidentally (along with a few other patches).

> I posted a more complete implementation a few weeks ago, anything wrong
> with it?
> 
> http://thread.gmane.org/gmane.comp.emulators.qemu/24567

People saying it is not the way to go, and I have still haven't decided
myself if it is a correct solution or not.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anders]

Aurelien Jarno wrote:

> That's why I reverted that commit, it was for my local tree only, I
> committed it accidentally (along with a few other patches).

Ah, okay. I only read the subject of your reverts, which did not mention 
4341.

>> I posted a more complete implementation a few weeks ago, anything wrong
>> with it?
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/24567
> 
> People saying it is not the way to go, and I have still haven't decided
> myself if it is a correct solution or not.
> 

The current code is also not a correct solution.

Well, I will just keep it in my local tree.


Cheers,
Anders.

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anthony Liguori]

Aurelien Jarno wrote:
> Revision: 4341
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4341
> Author:   aurel32
> Date:     2008-05-05 21:26:43 +0000 (Mon, 05 May 2008)
>
> Log Message:
> -----------
> qemu: generate signals on tap I/O
>
> Currently tap does not generate signals on I/O; this causes
> network latency to be dependent on the timer tick (1ms without
> dyntick, guest dependent with dyntick).  By generating a signal
> on I/O, we can inform the guest immediately that a packet has
> arrived.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>
> Modified Paths:
> --------------
>     trunk/vl.c
>
> Modified: trunk/vl.c
> ===================================================================
> --- trunk/vl.c	2008-05-05 21:26:31 UTC (rev 4340)
> +++ trunk/vl.c	2008-05-05 21:26:43 UTC (rev 4341)
> @@ -4030,6 +4030,7 @@
>      if (!s)
>          return NULL;
>      s->fd = fd;
> +    enable_sigio_timer(fd);
>      s->vc = qemu_new_vlan_client(vlan, tap_receive, NULL, s);
>      qemu_set_fd_handler(s->fd, tap_send, NULL, s);
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str), "tap: fd=%d", fd);
>   

I still haven't seen anyone explain why this results in a performance 
improvement.  The SIGIO handler is tied to the host_alarm_handler which 
will not dispatch IO.  It surprises me that it has any affect at all.

FWIW, we're getting rid of SIGIO in KVM.  It doesn't improve performance 
verses a properly implemented select lop.

Regards,

Anthony Liguori

>
>
>   

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anthony Liguori]

Anders wrote:
> Aurelien Jarno wrote:
>
>> That's why I reverted that commit, it was for my local tree only, I
>> committed it accidentally (along with a few other patches).
>
> Ah, okay. I only read the subject of your reverts, which did not 
> mention 4341.
>
>>> I posted a more complete implementation a few weeks ago, anything wrong
>>> with it?
>>>
>>> http://thread.gmane.org/gmane.comp.emulators.qemu/24567
>>
>> People saying it is not the way to go, and I have still haven't decided
>> myself if it is a correct solution or not.
>>
>
> The current code is also not a correct solution.
>
> Well, I will just keep it in my local tree.

You're seeing improvement with normal QEMU?  Can you please post a 
description of what you're seeing improve with SIGIO.  SIGIO should 
really only slow things down.

Regards,

Anthony Liguori

>
> Cheers,
> Anders.
>
>

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Aurelien Jarno]

Anthony Liguori a écrit :
> Anders wrote:
>> Aurelien Jarno wrote:
>>
>>> That's why I reverted that commit, it was for my local tree only, I
>>> committed it accidentally (along with a few other patches).
>>
>> Ah, okay. I only read the subject of your reverts, which did not
>> mention 4341.
>>
>>>> I posted a more complete implementation a few weeks ago, anything wrong
>>>> with it?
>>>>
>>>> http://thread.gmane.org/gmane.comp.emulators.qemu/24567
>>>
>>> People saying it is not the way to go, and I have still haven't decided
>>> myself if it is a correct solution or not.
>>>
>>
>> The current code is also not a correct solution.
>>
>> Well, I will just keep it in my local tree.
> 
> You're seeing improvement with normal QEMU?  Can you please post a
> description of what you're seeing improve with SIGIO.  SIGIO should
> really only slow things down.

I haven't tried Anders' patch, but the patch I accidentally committed
improved the network speed (tap mode) by a huge factor (I haven't the
exact factor in mind anymore). I can now easily reach 170 MBps with an
e1000 card on the MIPS Malta target.

That's why I use that patch in my local tree.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anthony Liguori]

Anders wrote:
> Aurelien Jarno wrote:
>
>> That's why I reverted that commit, it was for my local tree only, I
>> committed it accidentally (along with a few other patches).
>
> Ah, okay. I only read the subject of your reverts, which did not 
> mention 4341.
>
>>> I posted a more complete implementation a few weeks ago, anything wrong
>>> with it?
>>>
>>> http://thread.gmane.org/gmane.comp.emulators.qemu/24567
>>
>> People saying it is not the way to go, and I have still haven't decided
>> myself if it is a correct solution or not.
>>
>
> The current code is also not a correct solution.
>
> Well, I will just keep it in my local tree.

BTW, the biggest problem with tap networking in QEMU seems to be packet 
loss.  The fd_can_read handler is completely ignored for tap which means 
that if the device isn't ready to receive a packet, the packet is simply 
dropped.

I have a patch that fixes this but the only driver that has a proper 
fd_can_read handler right now is virtio-net.  I plan on including this 
tap fix in my next round of virtio patches.

Addressing the packet loss issue improves TCP streaming performance by 
4x.  In contrast, eliminating the copy in the virtio-net device only 
improves performance by about 10% (although it will probably be a lot 
more when tun/tap supports zero-copy).

Regards,

Anthony Liguori

>
> Cheers,
> Anders.
>
>

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Aurelien Jarno]

Aurelien Jarno a écrit :
> Anthony Liguori a écrit :
>> Anders wrote:
>>> Aurelien Jarno wrote:
>>>
>>>> That's why I reverted that commit, it was for my local tree only, I
>>>> committed it accidentally (along with a few other patches).
>>> Ah, okay. I only read the subject of your reverts, which did not
>>> mention 4341.
>>>
>>>>> I posted a more complete implementation a few weeks ago, anything wrong
>>>>> with it?
>>>>>
>>>>> http://thread.gmane.org/gmane.comp.emulators.qemu/24567
>>>> People saying it is not the way to go, and I have still haven't decided
>>>> myself if it is a correct solution or not.
>>>>
>>> The current code is also not a correct solution.
>>>
>>> Well, I will just keep it in my local tree.
>> You're seeing improvement with normal QEMU?  Can you please post a
>> description of what you're seeing improve with SIGIO.  SIGIO should
>> really only slow things down.
> 
> I haven't tried Anders' patch, but the patch I accidentally committed
> improved the network speed (tap mode) by a huge factor (I haven't the
> exact factor in mind anymore). I can now easily reach 170 MBps with an
                                                       ^^^^^^^^^^
                                                  oops that is 170 Mbps

> e1000 card on the MIPS Malta target.
> 
> That's why I use that patch in my local tree.
> 


-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anthony Liguori]

Aurelien Jarno wrote:
>>> You're seeing improvement with normal QEMU?  Can you please post a
>>> description of what you're seeing improve with SIGIO.  SIGIO should
>>> really only slow things down.
>>>       
>> I haven't tried Anders' patch, but the patch I accidentally committed
>> improved the network speed (tap mode) by a huge factor (I haven't the
>> exact factor in mind anymore). I can now easily reach 170 MBps with an
>>     
>                                                        ^^^^^^^^^^
>                                                   oops that is 170 Mbps
>   

If you explicitly call host_alarm_handler in tap_send(), do you see the 
same improvement?  Perhaps your host timer is just very slow?

Regards,

Anthony Liguori

>> e1000 card on the MIPS Malta target.
>>
>> That's why I use that patch in my local tree.
>>
>>     
>
>
>   

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anthony Liguori]

Anthony Liguori wrote:
>
> If you explicitly call host_alarm_handler in tap_send(), do you see 
> the same improvement?  Perhaps your host timer is just very slow?

Oh, it's probably that the signal is causing TCG to be interrupted and 
IO to be processed immediately.  Moving the select() loop into a 
separate thread that sets an atomic whenever it's time to break out of 
the TCG loop would be a more general solution.

Regards,

Anthony Liguori

> Regards,
>
> Anthony Liguori
>
>>> e1000 card on the MIPS Malta target.
>>>
>>> That's why I use that patch in my local tree.
>>>
>>>     
>>
>>
>>   
>

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anders]

Anthony Liguori wrote:
> You're seeing improvement with normal QEMU?  Can you please post a 
> description of what you're seeing improve with SIGIO.  SIGIO should 
> really only slow things down.
>   

I found this issue in KVM. I have not timed in QEMU, but the issue 
seemed generic, so I posted it upstream.

My usecase was a new VNC connection delaying until the next timer tick 
(which is a really long time in some KVM configurations).

The improvement is from running the select() immediately, rather than 
waiting around for a timer to expire.

Thanks,
Anders.

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anthony Liguori]

Anders wrote:
> Anthony Liguori wrote:
>> You're seeing improvement with normal QEMU?  Can you please post a 
>> description of what you're seeing improve with SIGIO.  SIGIO should 
>> really only slow things down.
>>   
>
> I found this issue in KVM. I have not timed in QEMU, but the issue 
> seemed generic, so I posted it upstream.

Right, this was an artifact of how KVM handled select.  It's not generic 
at all.

> My usecase was a new VNC connection delaying until the next timer tick 
> (which is a really long time in some KVM configurations).
>
> The improvement is from running the select() immediately, rather than 
> waiting around for a timer to expire.

In KVM today, select() is in it's own thread so it will service IO 
immediately.  This makes SIGIO unnecessary (not quite correct yet but 
will be very soon).

Regards,

Anthony Liguori

> Thanks,
> Anders.
>

Re: [Qemu-devel] [4341] qemu: generate signals on tap I/O

[Anders]

Anthony Liguori wrote:
> Anders wrote:
>> Anthony Liguori wrote:
>>> You're seeing improvement with normal QEMU?  Can you please post a
>>> description of what you're seeing improve with SIGIO.  SIGIO should
>>> really only slow things down.
>>>
>>
>> I found this issue in KVM. I have not timed in QEMU, but the issue
>> seemed generic, so I posted it upstream.
>
> Right, this was an artifact of how KVM handled select.  It's not generic
> at all.

If that is true, then the SIGALRM IO handler must also be redundant?


> In KVM today, select() is in it's own thread so it will service IO
> immediately.  This makes SIGIO unnecessary (not quite correct yet but
> will be very soon).

Yes, you keep saying that. I think that work on a new implementation is
orthogonal to fixing the current one, though.


Cheers,
Anders.