[Qemu-devel] Re: [PATCH 5/5] virtio-net: Switch default to new bottom half TX handler for iothread

[Anthony Liguori <anthony@codemonkey.ws>]

On 08/31/2010 05:32 PM, Alex Williamson wrote:
> On Tue, 2010-08-31 at 23:25 +0300, Michael S. Tsirkin wrote:
>    
>> On Fri, Aug 27, 2010 at 04:37:45PM -0600, Alex Williamson wrote:
>>      
>>> The bottom half handler shows big improvements over the timer
>>> with few downsides, default to it when the iothread is enabled.
>>>
>>> Using the following tests, with the guest and host connected
>>> via tap+bridge:
>>>
>>> guest>  netperf -t TCP_STREAM -H $HOST
>>> host>  netperf -t TCP_STREAM -H $GUEST
>>> guest>  netperf -t UDP_STREAM -H $HOST
>>> host>  netperf -t UDP_STREAM -H $GUEST
>>> guest>  netperf -t TCP_RR -H $HOST
>>>
>>> Results: base throughput, exits/throughput ->
>>>                     patched throughput, exits/throughput
>>>
>>> --enable-io-thread
>>> TCP guest->host 2737.77, 47.82  ->  6767.09, 29.15 = 247%, 61%
>>> TCP host->guest 2231.33, 74.00  ->  4125.80, 67.61 = 185%, 91%
>>> UDP guest->host 6281.68, 14.66  ->  12569.27, 1.98 = 200%, 14%
>>> UDP host->guest 275.91,  289.22 ->  264.80, 293.53 = 96%, 101%
>>> interations/s   1949.65, 82.97  ->  7417.56, 84.31 = 380%, 102%
>>>
>>> No --enable-io-thread
>>> TCP guest->host 3041.57, 55.11 ->  1038.93, 517.57 = 34%, 939%
>>> TCP host->guest 2416.03, 76.67 ->  5655.92, 55.52  = 234%, 72%
>>> UDP guest->host 12255.82, 6.11 ->  7775.87, 31.32  = 63%, 513%
>>> UDP host->guest 587.92, 245.95 ->  611.88, 239.92  = 104%, 98%
>>> interations/s   1975.59, 83.21 ->  8935.50, 88.18  = 452%, 106%
>>>
>>> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>>>        
>> parameter having different settings based on config
>> options might surprise some users. I don't think
>> we really need a parameter here ...
>>      
> I'm not a bit fan of this either, but I'd also prefer not to introduce a
> regression for a performance difference we know about in advance.  It
> gets even more complicated when we factor in qemu-kvm, as it doesn't
> build with iothread enabled, but seems to get and even better boost in
> performance across the board thanks largely to the kvm-irqchip.  Should
> we instead make this a configure option?  --enable-virtio-net-txbh?
>    

No, at this stage, we should ignore no --enable-io-thread with -enable-kvm.

Regards,

Anthony Liguori

> Thanks,
>
> Alex
>
>    
>>> ---
>>>
>>>   hw/s390-virtio-bus.c |    3 ++-
>>>   hw/syborg_virtio.c   |    3 ++-
>>>   hw/virtio-pci.c      |    3 ++-
>>>   hw/virtio.h          |    6 ++++++
>>>   4 files changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
>>> index 1483362..985f99a 100644
>>> --- a/hw/s390-virtio-bus.c
>>> +++ b/hw/s390-virtio-bus.c
>>> @@ -328,7 +328,8 @@ static VirtIOS390DeviceInfo s390_virtio_net = {
>>>       .qdev.size = sizeof(VirtIOS390Device),
>>>       .qdev.props = (Property[]) {
>>>           DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
>>> -        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer, 1),
>>> +        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer,
>>> +                           TXTIMER_DEFAULT),
>>>           DEFINE_PROP_INT32("txburst", VirtIOS390Device, txburst, 256),
>>>           DEFINE_PROP_END_OF_LIST(),
>>>       },
>>> diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
>>> index 7b76972..ee5746d 100644
>>> --- a/hw/syborg_virtio.c
>>> +++ b/hw/syborg_virtio.c
>>> @@ -300,7 +300,8 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
>>>       .qdev.props = (Property[]) {
>>>           DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
>>>           DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
>>> -        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer, 1),
>>> +        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer,
>>> +                           TXTIMER_DEFAULT),
>>>           DEFINE_PROP_INT32("txburst", SyborgVirtIOProxy, txburst, 256),
>>>           DEFINE_PROP_END_OF_LIST(),
>>>       }
>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>> index e025c09..9740f57 100644
>>> --- a/hw/virtio-pci.c
>>> +++ b/hw/virtio-pci.c
>>> @@ -695,7 +695,8 @@ static PCIDeviceInfo virtio_info[] = {
>>>               DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
>>>               DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
>>>               DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
>>> -            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer, 1),
>>> +            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer,
>>> +                               TXTIMER_DEFAULT),
>>>               DEFINE_PROP_INT32("txburst", VirtIOPCIProxy, txburst, 256),
>>>               DEFINE_PROP_END_OF_LIST(),
>>>           },
>>> diff --git a/hw/virtio.h b/hw/virtio.h
>>> index 4051889..a1a17a2 100644
>>> --- a/hw/virtio.h
>>> +++ b/hw/virtio.h
>>> @@ -183,6 +183,12 @@ void virtio_update_irq(VirtIODevice *vdev);
>>>   void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
>>>                           void *opaque);
>>>
>>> +#ifdef CONFIG_IOTHREAD
>>> + #define TXTIMER_DEFAULT 0
>>> +#else
>>> + #define TXTIMER_DEFAULT 1
>>> +#endif
>>> +
>>>        
>> Add a comment explaning that this is just a performance optimization?
>>
>>      
>>>   /* Base devices.  */
>>>   VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
>>>   VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>        
>
>
>