[Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Paolo Bonzini]

From: Vincenzo Maffione 

The virtqueue_pop() implementation needs to check if the avail ring
contains some pending buffers. To perform this check, it is not
always necessary to fetch the avail_idx in the VQ memory, which is
expensive. This patch introduces a shadow variable tracking avail_idx
and modifies virtio_queue_empty() to access avail_idx in physical
memory only when necessary.

Signed-off-by: Vincenzo Maffione 
Message-Id: 

Reviewed-by: Cornelia Huck 
Signed-off-by: Paolo Bonzini 
---
v1->v2: update shadow avail_idx in virtio_queue_set_last_avail_idx
[Michael]

 hw/virtio/virtio.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5116a2e..6842938 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -70,8 +70,13 @@ typedef struct VRing
 struct VirtQueue
 {
 VRing vring;
+
+/* Next head to pop */
 uint16_t last_avail_idx;
 
+/* Last avail_idx read from VQ. */
+uint16_t shadow_avail_idx;
+
 uint16_t used_idx;
 
 /* Last used index value we have signalled on */
@@ -132,7 +137,8 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
 hwaddr pa;
 pa = vq->vring.avail + offsetof(VRingAvail, idx);
-return virtio_lduw_phys(vq->vdev, pa);
+vq->shadow_avail_idx = virtio_lduw_phys(vq->vdev, pa);
+return vq->shadow_avail_idx;
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
@@ -223,8 +229,14 @@ int virtio_queue_ready(VirtQueue *vq)
 return vq->vring.avail != 0;
 }
 
+/* Fetch avail_idx from VQ memory only when we really need to know if
+ * guest has added some buffers. */
 int virtio_queue_empty(VirtQueue *vq)
 {
+if (vq->shadow_avail_idx != vq->last_avail_idx) {
+return 0;
+}
+
 return vring_avail_idx(vq) == vq->last_avail_idx;
 }
 
@@ -300,7 +312,7 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int 
idx)
 /* Check it isn't doing very strange things with descriptor numbers. */
 if (num_heads > vq->vring.num) {
 error_report("Guest moved used index from %u to %u",
- idx, vring_avail_idx(vq));
+ idx, vq->shadow_avail_idx);
 exit(1);
 }
 /* On success, callers read a descriptor at vq->last_avail_idx.
@@ -535,9 +547,12 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 struct iovec iov[VIRTQUEUE_MAX_SIZE];
 VRingDesc desc;
 
-if (!virtqueue_num_heads(vq, vq->last_avail_idx)) {
+if (virtio_queue_empty(vq)) {
 return NULL;
 }
+/* Needed after virtio_queue_empty(), see comment in
+ * virtqueue_num_heads(). */
+smp_rmb();
 
 /* When we start there are none of either input nor output. */
 out_num = in_num = 0;
@@ -786,6 +801,7 @@ void virtio_reset(void *opaque)
 vdev->vq[i].vring.avail = 0;
 vdev->vq[i].vring.used = 0;
 vdev->vq[i].last_avail_idx = 0;
+vdev->vq[i].shadow_avail_idx = 0;
 vdev->vq[i].used_idx = 0;
 virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
 vdev->vq[i].signalled_used = 0;
@@ -1155,7 +1171,7 @@ static bool vring_notify(VirtIODevice *vdev, VirtQueue 
*vq)
 smp_mb();
 /* Always notify when queue is empty (when feature acknowledge) */
 if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
-!vq->inuse && vring_avail_idx(vq) == vq->last_avail_idx) {
+!vq->inuse && virtio_queue_empty(vq)) {
 return true;
 }
 
@@ -1579,6 +1595,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
 return -1;
 }
 vdev->vq[i].used_idx = vring_used_idx(>vq[i]);
+vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
 }
 }
 
@@ -1714,6 +1731,7 @@ uint16_t virtio_queue_get_last_avail_idx(VirtIODevice 
*vdev, int n)
 void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx)
 {
 vdev->vq[n].last_avail_idx = idx;
+vdev->vq[n].shadow_avail_idx = idx;
 }
 
 void virtio_queue_invalidate_signalled_used(VirtIODevice *vdev, int n)
-- 
2.5.0

Re: [Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Vincenzo Maffione]

2016-01-19 19:48 GMT+01:00 Paolo Bonzini :
>
>
> On 19/01/2016 17:54, Michael S. Tsirkin wrote:
>> On Fri, Jan 15, 2016 at 01:41:57PM +0100, Paolo Bonzini wrote:
>>> From: Vincenzo Maffione 
>>>
>>> The virtqueue_pop() implementation needs to check if the avail ring
>>> contains some pending buffers. To perform this check, it is not
>>> always necessary to fetch the avail_idx in the VQ memory, which is
>>> expensive. This patch introduces a shadow variable tracking avail_idx
>>> and modifies virtio_queue_empty() to access avail_idx in physical
>>> memory only when necessary.
>>>
>>> Signed-off-by: Vincenzo Maffione 
>>> Message-Id: 
>>> 
>>> Signed-off-by: Paolo Bonzini 
>>
>> Is the cost due to the page walk?
>
> Yes, as with all the other patches.  But unlike patches 7 and 10 where
> we just reduce the number of walks, for patch 8 and 9 it's difficult to
> beat a local cache. :)
>
>>> @@ -1579,6 +1595,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
>>> version_id)
>>>  return -1;
>>>  }
>>>  vdev->vq[i].used_idx = vring_used_idx(>vq[i]);
>>> +vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
>>>  }
>>>  }
>>
>>
>> shadow_avail_idx also should be updated on vhost stop,
>
> That's virtio_queue_set_last_avail_idx, right?
>
> Paolo

Right. Sorry, I missed that.

Vincenzo



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Cornelia Huck]

On Tue, 19 Jan 2016 19:48:42 +0100
Paolo Bonzini  wrote:

> >> @@ -1579,6 +1595,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
> >> version_id)
> >>  return -1;
> >>  }
> >>  vdev->vq[i].used_idx = vring_used_idx(>vq[i]);
> >> +vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
> >>  }
> >>  }
> > 
> > 
> > shadow_avail_idx also should be updated on vhost stop,
> 
> That's virtio_queue_set_last_avail_idx, right?

That should also take care of vring teardown.

Re: [Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Paolo Bonzini]

On 19/01/2016 17:54, Michael S. Tsirkin wrote:
> On Fri, Jan 15, 2016 at 01:41:57PM +0100, Paolo Bonzini wrote:
>> From: Vincenzo Maffione 
>>
>> The virtqueue_pop() implementation needs to check if the avail ring
>> contains some pending buffers. To perform this check, it is not
>> always necessary to fetch the avail_idx in the VQ memory, which is
>> expensive. This patch introduces a shadow variable tracking avail_idx
>> and modifies virtio_queue_empty() to access avail_idx in physical
>> memory only when necessary.
>>
>> Signed-off-by: Vincenzo Maffione 
>> Message-Id: 
>> 
>> Signed-off-by: Paolo Bonzini 
> 
> Is the cost due to the page walk?

Yes, as with all the other patches.  But unlike patches 7 and 10 where
we just reduce the number of walks, for patch 8 and 9 it's difficult to
beat a local cache. :)

>> @@ -1579,6 +1595,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
>> version_id)
>>  return -1;
>>  }
>>  vdev->vq[i].used_idx = vring_used_idx(>vq[i]);
>> +vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
>>  }
>>  }
> 
> 
> shadow_avail_idx also should be updated on vhost stop,

That's virtio_queue_set_last_avail_idx, right?

Paolo

Re: [Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Cornelia Huck]

On Fri, 15 Jan 2016 13:41:57 +0100
Paolo Bonzini  wrote:

> From: Vincenzo Maffione 
> 
> The virtqueue_pop() implementation needs to check if the avail ring
> contains some pending buffers. To perform this check, it is not
> always necessary to fetch the avail_idx in the VQ memory, which is
> expensive. This patch introduces a shadow variable tracking avail_idx
> and modifies virtio_queue_empty() to access avail_idx in physical
> memory only when necessary.
> 
> Signed-off-by: Vincenzo Maffione 
> Message-Id: 
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/virtio/virtio.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Michael S. Tsirkin]

On Fri, Jan 15, 2016 at 01:41:57PM +0100, Paolo Bonzini wrote:
> From: Vincenzo Maffione 
> 
> The virtqueue_pop() implementation needs to check if the avail ring
> contains some pending buffers. To perform this check, it is not
> always necessary to fetch the avail_idx in the VQ memory, which is
> expensive. This patch introduces a shadow variable tracking avail_idx
> and modifies virtio_queue_empty() to access avail_idx in physical
> memory only when necessary.
> 
> Signed-off-by: Vincenzo Maffione 
> Message-Id: 
> 
> Signed-off-by: Paolo Bonzini 

Is the cost due to the page walk?

> ---
>  hw/virtio/virtio.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3e7c6bf..01142da 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -70,8 +70,13 @@ typedef struct VRing
>  struct VirtQueue
>  {
>  VRing vring;
> +
> +/* Next head to pop */
>  uint16_t last_avail_idx;
>  
> +/* Last avail_idx read from VQ. */
> +uint16_t shadow_avail_idx;
> +
>  uint16_t used_idx;
>  
>  /* Last used index value we have signalled on */
> @@ -132,7 +137,8 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
>  {
>  hwaddr pa;
>  pa = vq->vring.avail + offsetof(VRingAvail, idx);
> -return virtio_lduw_phys(vq->vdev, pa);
> +vq->shadow_avail_idx = virtio_lduw_phys(vq->vdev, pa);
> +return vq->shadow_avail_idx;
>  }
>  
>  static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
> @@ -223,8 +229,14 @@ int virtio_queue_ready(VirtQueue *vq)
>  return vq->vring.avail != 0;
>  }
>  
> +/* Fetch avail_idx from VQ memory only when we really need to know if
> + * guest has added some buffers. */
>  int virtio_queue_empty(VirtQueue *vq)
>  {
> +if (vq->shadow_avail_idx != vq->last_avail_idx) {
> +return 0;
> +}
> +
>  return vring_avail_idx(vq) == vq->last_avail_idx;
>  }
>  
> @@ -300,7 +312,7 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned 
> int idx)
>  /* Check it isn't doing very strange things with descriptor numbers. */
>  if (num_heads > vq->vring.num) {
>  error_report("Guest moved used index from %u to %u",
> - idx, vring_avail_idx(vq));
> + idx, vq->shadow_avail_idx);
>  exit(1);
>  }
>  /* On success, callers read a descriptor at vq->last_avail_idx.
> @@ -534,9 +546,12 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>  struct iovec iov[VIRTQUEUE_MAX_SIZE];
>  VRingDesc desc;
>  
> -if (!virtqueue_num_heads(vq, vq->last_avail_idx)) {
> +if (virtio_queue_empty(vq)) {
>  return NULL;
>  }
> +/* Needed after virtio_queue_empty(), see comment in
> + * virtqueue_num_heads(). */
> +smp_rmb();
>  
>  /* When we start there are none of either input nor output. */
>  out_num = in_num = 0;
> @@ -786,6 +801,7 @@ void virtio_reset(void *opaque)
>  vdev->vq[i].vring.avail = 0;
>  vdev->vq[i].vring.used = 0;
>  vdev->vq[i].last_avail_idx = 0;
> +vdev->vq[i].shadow_avail_idx = 0;
>  vdev->vq[i].used_idx = 0;
>  virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
>  vdev->vq[i].signalled_used = 0;
> @@ -1155,7 +1171,7 @@ static bool vring_notify(VirtIODevice *vdev, VirtQueue 
> *vq)
>  smp_mb();
>  /* Always notify when queue is empty (when feature acknowledge) */
>  if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
> -!vq->inuse && vring_avail_idx(vq) == vq->last_avail_idx) {
> +!vq->inuse && virtio_queue_empty(vq)) {
>  return true;
>  }
>  
> @@ -1579,6 +1595,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
> version_id)
>  return -1;
>  }
>  vdev->vq[i].used_idx = vring_used_idx(>vq[i]);
> +vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
>  }
>  }


shadow_avail_idx also should be updated on vhost stop,


>  
> -- 
> 2.5.0
> 

[Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Paolo Bonzini]

From: Vincenzo Maffione 

The virtqueue_pop() implementation needs to check if the avail ring
contains some pending buffers. To perform this check, it is not
always necessary to fetch the avail_idx in the VQ memory, which is
expensive. This patch introduces a shadow variable tracking avail_idx
and modifies virtio_queue_empty() to access avail_idx in physical
memory only when necessary.

Signed-off-by: Vincenzo Maffione 
Message-Id: 

Signed-off-by: Paolo Bonzini 
---
 hw/virtio/virtio.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3e7c6bf..01142da 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -70,8 +70,13 @@ typedef struct VRing
 struct VirtQueue
 {
 VRing vring;
+
+/* Next head to pop */
 uint16_t last_avail_idx;
 
+/* Last avail_idx read from VQ. */
+uint16_t shadow_avail_idx;
+
 uint16_t used_idx;
 
 /* Last used index value we have signalled on */
@@ -132,7 +137,8 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
 hwaddr pa;
 pa = vq->vring.avail + offsetof(VRingAvail, idx);
-return virtio_lduw_phys(vq->vdev, pa);
+vq->shadow_avail_idx = virtio_lduw_phys(vq->vdev, pa);
+return vq->shadow_avail_idx;
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
@@ -223,8 +229,14 @@ int virtio_queue_ready(VirtQueue *vq)
 return vq->vring.avail != 0;
 }
 
+/* Fetch avail_idx from VQ memory only when we really need to know if
+ * guest has added some buffers. */
 int virtio_queue_empty(VirtQueue *vq)
 {
+if (vq->shadow_avail_idx != vq->last_avail_idx) {
+return 0;
+}
+
 return vring_avail_idx(vq) == vq->last_avail_idx;
 }
 
@@ -300,7 +312,7 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int 
idx)
 /* Check it isn't doing very strange things with descriptor numbers. */
 if (num_heads > vq->vring.num) {
 error_report("Guest moved used index from %u to %u",
- idx, vring_avail_idx(vq));
+ idx, vq->shadow_avail_idx);
 exit(1);
 }
 /* On success, callers read a descriptor at vq->last_avail_idx.
@@ -534,9 +546,12 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 struct iovec iov[VIRTQUEUE_MAX_SIZE];
 VRingDesc desc;
 
-if (!virtqueue_num_heads(vq, vq->last_avail_idx)) {
+if (virtio_queue_empty(vq)) {
 return NULL;
 }
+/* Needed after virtio_queue_empty(), see comment in
+ * virtqueue_num_heads(). */
+smp_rmb();
 
 /* When we start there are none of either input nor output. */
 out_num = in_num = 0;
@@ -786,6 +801,7 @@ void virtio_reset(void *opaque)
 vdev->vq[i].vring.avail = 0;
 vdev->vq[i].vring.used = 0;
 vdev->vq[i].last_avail_idx = 0;
+vdev->vq[i].shadow_avail_idx = 0;
 vdev->vq[i].used_idx = 0;
 virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
 vdev->vq[i].signalled_used = 0;
@@ -1155,7 +1171,7 @@ static bool vring_notify(VirtIODevice *vdev, VirtQueue 
*vq)
 smp_mb();
 /* Always notify when queue is empty (when feature acknowledge) */
 if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
-!vq->inuse && vring_avail_idx(vq) == vq->last_avail_idx) {
+!vq->inuse && virtio_queue_empty(vq)) {
 return true;
 }
 
@@ -1579,6 +1595,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
 return -1;
 }
 vdev->vq[i].used_idx = vring_used_idx(>vq[i]);
+vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
 }
 }
 
-- 
2.5.0