[Qemu-devel] [PATCH v2] hw/virtio/vring/event_idx: fix the vring_avail_event error

[Qemu-devel] [PATCH v2] hw/virtio/vring/event_idx: fix the vring_avail_event error

[Bin Wu]

The event idx in virtio is an effective way to reduce the number of
interrupts and exits of the guest. When the guest puts an request
into the virtio ring, it doesn't exit immediately to inform the
backend. Instead, the guest checks the "avail" event idx to determine
the notification.

In virtqueue_pop, when a request is poped, the current avail event
idx should be set to the number of vq->last_avail_idx.

Signed-off-by: Bin Wu <wu.wubin@huawei.com>
---
V2 -> V1:
update the same code in hw/virtio/dataplane/vring.c (Stefan)
---
 hw/virtio/dataplane/vring.c | 8 ++++----
 hw/virtio/virtio.c          | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 372706a..61f6d83 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -352,10 +352,6 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
         goto out;
     }
 
-    if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
-        vring_avail_event(&vring->vr) = vring->vr.avail->idx;
-    }
-
     i = head;
     do {
         if (unlikely(i >= num)) {
@@ -392,6 +388,10 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
 
     /* On success, increment avail index. */
     vring->last_avail_idx++;
+    if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
+        vring_avail_event(&vring->vr) = vring->last_avail_idx;
+    }
+
     return head;
 
 out:
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2c236bf..013979a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -469,7 +469,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 
     i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
     if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
-        vring_avail_event(vq, vring_avail_idx(vq));
+        vring_avail_event(vq, vq->last_avail_idx);
     }
 
     if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2] hw/virtio/vring/event_idx: fix the vring_avail_event error

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On Fri, Oct 31, 2014 at 12:40:16AM +0000, Bin Wu wrote:
> The event idx in virtio is an effective way to reduce the number of
> interrupts and exits of the guest. When the guest puts an request
> into the virtio ring, it doesn't exit immediately to inform the
> backend. Instead, the guest checks the "avail" event idx to determine
> the notification.
> 
> In virtqueue_pop, when a request is poped, the current avail event
> idx should be set to the number of vq->last_avail_idx.
> 
> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
> ---
> V2 -> V1:
> update the same code in hw/virtio/dataplane/vring.c (Stefan)
> ---
>  hw/virtio/dataplane/vring.c | 8 ++++----
>  hw/virtio/virtio.c          | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

By the way, did you see that kernel drivers/vhost/vhost.c doesn't update
the field for each pop?  Instead it only updates when notify is
re-enabled.  I wonder if that approach is better than what QEMU does.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2] hw/virtio/vring/event_idx: fix the vring_avail_event error

[Michael S. Tsirkin]

On Fri, Oct 31, 2014 at 10:57:55AM +0000, Stefan Hajnoczi wrote:
> On Fri, Oct 31, 2014 at 12:40:16AM +0000, Bin Wu wrote:
> > The event idx in virtio is an effective way to reduce the number of
> > interrupts and exits of the guest. When the guest puts an request
> > into the virtio ring, it doesn't exit immediately to inform the
> > backend. Instead, the guest checks the "avail" event idx to determine
> > the notification.
> > 
> > In virtqueue_pop, when a request is poped, the current avail event
> > idx should be set to the number of vq->last_avail_idx.
> > 
> > Signed-off-by: Bin Wu <wu.wubin@huawei.com>
> > ---
> > V2 -> V1:
> > update the same code in hw/virtio/dataplane/vring.c (Stefan)
> > ---
> >  hw/virtio/dataplane/vring.c | 8 ++++----
> >  hw/virtio/virtio.c          | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> By the way, did you see that kernel drivers/vhost/vhost.c doesn't update
> the field for each pop?  Instead it only updates when notify is
> re-enabled.  I wonder if that approach is better than what QEMU does.
> 
> Stefan

Yes you should not move event idx if you don't want events.