[Qemu-devel] [PATCH] hw/virtio/virtio.c: fix the vring_avail_event error

[Qemu-devel] [PATCH] hw/virtio/virtio.c: 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>
---
 hw/virtio/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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] hw/virtio/virtio.c: fix the vring_avail_event error

[Michael S. Tsirkin]

On Tue, Oct 28, 2014 at 02:13:02AM +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>

Does this fix some observable bug? Improve efficiency for some workload?

> ---
>  hw/virtio/virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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] hw/virtio/virtio.c: fix the vring_avail_event error

[Bin Wu]

On 2014/10/28 13:32, Michael S. Tsirkin wrote:
> On Tue, Oct 28, 2014 at 02:13:02AM +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>
> 
> Does this fix some observable bug? Improve efficiency for some workload?
>
Improve efficiency, I test as follows:
(1)create a suse11sp3 VM with a virtio-blk test disk;
(2)use libaio library to submit 32 IOs concurrently (invoke io_sumbmit 32 times)
in the VM;
(3)modify the virtio-blk driver in the guest to record the host notification times;

without this patch, virtio-blk may notify the host between 10 and 30 times;
with this patch, virtio-blk just notify the host once during every test:)

>> ---
>>  hw/virtio/virtio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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
> 
> .
> 

-- 
Bin Wu

Re: [Qemu-devel] [PATCH] hw/virtio/virtio.c: fix the vring_avail_event error

[Stefan Hajnoczi]

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

On Tue, Oct 28, 2014 at 02:13:02AM +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>
> ---
>  hw/virtio/virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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);
>      }

hw/virtio/dataplane/vring.c:

  if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
      vring_avail_event(&vring->vr) = vring->vr.avail->idx;
  }

This is equivalent to the virtio.c code you are replacing, so it should
be updated too.

Stefan

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

Re: [Qemu-devel] [PATCH] hw/virtio/virtio.c: fix the vring_avail_event error

[Bin Wu]

On 2014/10/31 0:48, Stefan Hajnoczi wrote:
> On Tue, Oct 28, 2014 at 02:13:02AM +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>
>> ---
>>  hw/virtio/virtio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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);
>>      }
> 
> hw/virtio/dataplane/vring.c:
> 
>   if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
>       vring_avail_event(&vring->vr) = vring->vr.avail->idx;
>   }
> 
> This is equivalent to the virtio.c code you are replacing, so it should
> be updated too.
> 
> Stefan
> 
Thanks, stefan. I'll send another patch later.
-- 
Bin Wu