diff mbox series

[5/5] virtio: enable VIRTIO_F_RING_PACKED for all devices

Message ID 20200522171726.648279-6-stefanha@redhat.com (mailing list archive)
State New, archived
Headers show
Series virtio: enable VIRTIO_F_RING_PACKED for all devices | expand

Commit Message

Stefan Hajnoczi May 22, 2020, 5:17 p.m. UTC 
The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
ring instead of a split avail/used ring design. There are CPU cache
advantages to this layout and it is also suited better to hardware
implementation.

The vhost-net backend has already supported packed virtqueues for some
time. Performance benchmarks show that virtio-blk performance on NVMe
drives is also improved.

Go ahead and enable this feature for all VIRTIO devices. Keep it
disabled for QEMU 5.0 and earlier machine types.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio.h |  2 +-
 hw/core/machine.c          | 18 +++++++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

Comments

Dr. David Alan Gilbert May 26, 2020, 5:29 p.m. UTC | #1 
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
> ring instead of a split avail/used ring design. There are CPU cache
> advantages to this layout and it is also suited better to hardware
> implementation.
> 
> The vhost-net backend has already supported packed virtqueues for some
> time. Performance benchmarks show that virtio-blk performance on NVMe
> drives is also improved.
> 
> Go ahead and enable this feature for all VIRTIO devices. Keep it
> disabled for QEMU 5.0 and earlier machine types.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/hw/virtio/virtio.h |  2 +-
>  hw/core/machine.c          | 18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index b69d517496..fd5b4a2044 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>      DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
>                        VIRTIO_F_IOMMU_PLATFORM, false), \
>      DEFINE_PROP_BIT64("packed", _state, _field, \
> -                      VIRTIO_F_RING_PACKED, false)
> +                      VIRTIO_F_RING_PACKED, true)
>  
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>  bool virtio_queue_enabled(VirtIODevice *vdev, int n);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index bb3a7b18b1..3598c3c825 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,7 +28,23 @@
>  #include "hw/mem/nvdimm.h"
>  #include "migration/vmstate.h"
>  
> -GlobalProperty hw_compat_5_0[] = {};
> +GlobalProperty hw_compat_5_0[] = {
> +    { "vhost-user-blk", "packed", "off" },
> +    { "vhost-user-fs-device", "packed", "off" },
> +    { "vhost-vsock-device", "packed", "off" },
> +    { "virtio-9p-device", "packed", "off" },
> +    { "virtio-balloon-device", "packed", "off" },
> +    { "virtio-blk-device", "packed", "off" },
> +    { "virtio-crypto-device", "packed", "off" },
> +    { "virtio-gpu-device", "packed", "off" },
> +    { "virtio-input-device", "packed", "off" },
> +    { "virtio-iommu-device", "packed", "off" },
> +    { "virtio-net-device", "packed", "off" },
> +    { "virtio-pmem", "packed", "off" },
> +    { "virtio-rng-device", "packed", "off" },
> +    { "virtio-scsi-common", "packed", "off" },
> +    { "virtio-serial-device", "packed", "off" },
> +};
>  const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>  
>  GlobalProperty hw_compat_4_2[] = {
> -- 
> 2.25.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Jason Wang May 29, 2020, 7:15 a.m. UTC | #2 
On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
> The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
> ring instead of a split avail/used ring design. There are CPU cache
> advantages to this layout and it is also suited better to hardware
> implementation.
>
> The vhost-net backend has already supported packed virtqueues for some
> time. Performance benchmarks show that virtio-blk performance on NVMe
> drives is also improved.
>
> Go ahead and enable this feature for all VIRTIO devices. Keep it
> disabled for QEMU 5.0 and earlier machine types.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/hw/virtio/virtio.h |  2 +-
>   hw/core/machine.c          | 18 +++++++++++++++++-
>   2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index b69d517496..fd5b4a2044 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>       DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
>                         VIRTIO_F_IOMMU_PLATFORM, false), \
>       DEFINE_PROP_BIT64("packed", _state, _field, \
> -                      VIRTIO_F_RING_PACKED, false)
> +                      VIRTIO_F_RING_PACKED, true)
>   
>   hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>   bool virtio_queue_enabled(VirtIODevice *vdev, int n);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index bb3a7b18b1..3598c3c825 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,7 +28,23 @@
>   #include "hw/mem/nvdimm.h"
>   #include "migration/vmstate.h"
>   
> -GlobalProperty hw_compat_5_0[] = {};
> +GlobalProperty hw_compat_5_0[] = {
> +    { "vhost-user-blk", "packed", "off" },
> +    { "vhost-user-fs-device", "packed", "off" },
> +    { "vhost-vsock-device", "packed", "off" },
> +    { "virtio-9p-device", "packed", "off" },
> +    { "virtio-balloon-device", "packed", "off" },
> +    { "virtio-blk-device", "packed", "off" },
> +    { "virtio-crypto-device", "packed", "off" },
> +    { "virtio-gpu-device", "packed", "off" },
> +    { "virtio-input-device", "packed", "off" },
> +    { "virtio-iommu-device", "packed", "off" },
> +    { "virtio-net-device", "packed", "off" },
> +    { "virtio-pmem", "packed", "off" },
> +    { "virtio-rng-device", "packed", "off" },
> +    { "virtio-scsi-common", "packed", "off" },
> +    { "virtio-serial-device", "packed", "off" },


Missing "vhost-user-gpu" here?

I try to do something like this in the past but give up since I end up 
with similar list.

It would be better to consider something more smart, probably need some 
refactor for a common parent class.

Thanks


> +};
>   const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>   
>   GlobalProperty hw_compat_4_2[] = {
Stefan Hajnoczi May 29, 2020, 3:28 p.m. UTC | #3 
On Fri, May 29, 2020 at 03:15:59PM +0800, Jason Wang wrote:
> 
> On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
> > The packed virtqueue layout was introduced in VIRTIO 1.1. It is a single
> > ring instead of a split avail/used ring design. There are CPU cache
> > advantages to this layout and it is also suited better to hardware
> > implementation.
> > 
> > The vhost-net backend has already supported packed virtqueues for some
> > time. Performance benchmarks show that virtio-blk performance on NVMe
> > drives is also improved.
> > 
> > Go ahead and enable this feature for all VIRTIO devices. Keep it
> > disabled for QEMU 5.0 and earlier machine types.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   include/hw/virtio/virtio.h |  2 +-
> >   hw/core/machine.c          | 18 +++++++++++++++++-
> >   2 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index b69d517496..fd5b4a2044 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -292,7 +292,7 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> >       DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
> >                         VIRTIO_F_IOMMU_PLATFORM, false), \
> >       DEFINE_PROP_BIT64("packed", _state, _field, \
> > -                      VIRTIO_F_RING_PACKED, false)
> > +                      VIRTIO_F_RING_PACKED, true)
> >   hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> >   bool virtio_queue_enabled(VirtIODevice *vdev, int n);
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index bb3a7b18b1..3598c3c825 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -28,7 +28,23 @@
> >   #include "hw/mem/nvdimm.h"
> >   #include "migration/vmstate.h"
> > -GlobalProperty hw_compat_5_0[] = {};
> > +GlobalProperty hw_compat_5_0[] = {
> > +    { "vhost-user-blk", "packed", "off" },
> > +    { "vhost-user-fs-device", "packed", "off" },
> > +    { "vhost-vsock-device", "packed", "off" },
> > +    { "virtio-9p-device", "packed", "off" },
> > +    { "virtio-balloon-device", "packed", "off" },
> > +    { "virtio-blk-device", "packed", "off" },
> > +    { "virtio-crypto-device", "packed", "off" },
> > +    { "virtio-gpu-device", "packed", "off" },
> > +    { "virtio-input-device", "packed", "off" },
> > +    { "virtio-iommu-device", "packed", "off" },
> > +    { "virtio-net-device", "packed", "off" },
> > +    { "virtio-pmem", "packed", "off" },
> > +    { "virtio-rng-device", "packed", "off" },
> > +    { "virtio-scsi-common", "packed", "off" },
> > +    { "virtio-serial-device", "packed", "off" },
> 
> 
> Missing "vhost-user-gpu" here?

Thanks, you're right.

I'll see if virtio-gpu-base works. If not it will be necessary to add
all the derived classes. The same is true for virtio-scsi-common, I'd
better check it works correctly!

Stefan
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b69d517496..fd5b4a2044 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -292,7 +292,7 @@  typedef struct VirtIORNGConf VirtIORNGConf;
     DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
                       VIRTIO_F_IOMMU_PLATFORM, false), \
     DEFINE_PROP_BIT64("packed", _state, _field, \
-                      VIRTIO_F_RING_PACKED, false)
+                      VIRTIO_F_RING_PACKED, true)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 bool virtio_queue_enabled(VirtIODevice *vdev, int n);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index bb3a7b18b1..3598c3c825 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,7 +28,23 @@ 
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
 
-GlobalProperty hw_compat_5_0[] = {};
+GlobalProperty hw_compat_5_0[] = {
+    { "vhost-user-blk", "packed", "off" },
+    { "vhost-user-fs-device", "packed", "off" },
+    { "vhost-vsock-device", "packed", "off" },
+    { "virtio-9p-device", "packed", "off" },
+    { "virtio-balloon-device", "packed", "off" },
+    { "virtio-blk-device", "packed", "off" },
+    { "virtio-crypto-device", "packed", "off" },
+    { "virtio-gpu-device", "packed", "off" },
+    { "virtio-input-device", "packed", "off" },
+    { "virtio-iommu-device", "packed", "off" },
+    { "virtio-net-device", "packed", "off" },
+    { "virtio-pmem", "packed", "off" },
+    { "virtio-rng-device", "packed", "off" },
+    { "virtio-scsi-common", "packed", "off" },
+    { "virtio-serial-device", "packed", "off" },
+};
 const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
 
 GlobalProperty hw_compat_4_2[] = {