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

2010-09-01 Thread Stefan Hajnoczi
On Tue, Aug 31, 2010 at 11:32 PM, Alex Williamson
alex.william...@redhat.com 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.william...@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?
 Thanks,

 Alex

qemu-kvm uses its own iothread implementation by default.  It doesn't
need --enable-io-thread because it already uses a similar model.

Stefan


  ---
 
   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 

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

2010-09-01 Thread Michael S. Tsirkin
On Tue, Aug 31, 2010 at 04:32:54PM -0600, 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.william...@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.

I agree. The comment was mainly about a parameter. If we make it a hidden
parameter (mainbe with x- prefix), this won't be an issue.

 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?

That'll work too.

 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 

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

2010-08-31 Thread Michael S. Tsirkin
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.william...@redhat.com

parameter having different settings based on config
options might surprise some users. I don't think
we really need a parameter here ...

 ---
 
  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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2010-08-31 Thread Alex Williamson
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.william...@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?
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 

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

2010-08-31 Thread Anthony Liguori

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 Williamsonalex.william...@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 

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

2010-08-27 Thread Alex Williamson
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.william...@redhat.com
---

 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
+
 /* 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html