[PATCH] virtio_ring: Make interrupt suppression spec compliant

[PATCH] virtio_ring: Make interrupt suppression spec compliant

[Ladi Prosek]

According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
negotiated the driver MUST set flags to 0. Not dirtying the available
ring in virtqueue_disable_cb may also have a positive performance impact.

Writes to the used event field (vring_used_event) are still unconditional.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 drivers/virtio/virtio_ring.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ca6bfdd..d6345e1 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -718,7 +718,9 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
 
 	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
 		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
-		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+		if (!vq->event) {
+			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+		}
 	}
 
 }
@@ -750,7 +752,9 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
 	 * entry. Always do both to keep code simple. */
 	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
 		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
-		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+		if (!vq->event) {
+			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+		}
 	}
 	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
 	END_USE(vq);
@@ -818,10 +822,12 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
 	 * more to do. */
 	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
-	 * entry. Always do both to keep code simple. */
+	 * entry. Always update the event index to keep code simple. */
 	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
 		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
-		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+		if (!vq->event) {
+			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+		}
 	}
 	/* TODO: tune this threshold */
 	bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
@@ -939,7 +945,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback) {
 		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
-		vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
+		if (!vq->event) {
+			vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
+		}
 	}
 
 	/* Put everything in free lists. */
-- 
2.5.5

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Michael S. Tsirkin]

On Mon, Jun 06, 2016 at 11:51:34AM +0200, Ladi Prosek wrote:
> According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
> negotiated the driver MUST set flags to 0. Not dirtying the available
> ring in virtqueue_disable_cb may also have a positive performance impact.

Question would be, is this a gain or a loss. We have an extra branch,
and the write might serve to prefetch the cache line.


> Writes to the used event field (vring_used_event) are still unconditional.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>

Wow you are right wrt the spec. Should we change the spec or the
code though? Could you please test the performance a bit?

> ---
>  drivers/virtio/virtio_ring.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index ca6bfdd..d6345e1 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -718,7 +718,9 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
>  
>  	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
>  		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  
>  }
> @@ -750,7 +752,9 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>  	 * entry. Always do both to keep code simple. */
>  	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>  		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
>  	END_USE(vq);
> @@ -818,10 +822,12 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>  	 * more to do. */
>  	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
>  	 * either clear the flags bit or point the event index at the next
> -	 * entry. Always do both to keep code simple. */
> +	 * entry. Always update the event index to keep code simple. */
>  	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>  		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  	/* TODO: tune this threshold */
>  	bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
> @@ -939,7 +945,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>  	/* No callback?  Tell other side not to bother us. */
>  	if (!callback) {
>  		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
> +		}
>  	}


Linux coding style requires no {} around single statements.

>  
>  	/* Put everything in free lists. */
> -- 

I would appreciate it if you copy the correct mailing lists in the
future. Look  it up in MAINTAINERS. For this patch it is:
virtualization@lists.linux-foundation.org

Thanks!

> 2.5.5

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Paolo Bonzini]

On 06/06/2016 15:55, Michael S. Tsirkin wrote:
> > According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
> > negotiated the driver MUST set flags to 0. Not dirtying the available
> > ring in virtqueue_disable_cb may also have a positive performance impact.
> 
> Question would be, is this a gain or a loss. We have an extra branch,
> and the write might serve to prefetch the cache line.
> 
> > Writes to the used event field (vring_used_event) are still unconditional.
> > 
> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> 
> Wow you are right wrt the spec. Should we change the spec or the
> code though?

I would change the spec and note that bit 0 of the flags is ignored if
event indices are in use.

Paolo

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Ladi Prosek]

On Mon, Jun 6, 2016 at 4:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/06/2016 15:55, Michael S. Tsirkin wrote:
>> > According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
>> > negotiated the driver MUST set flags to 0. Not dirtying the available
>> > ring in virtqueue_disable_cb may also have a positive performance impact.
>>
>> Question would be, is this a gain or a loss. We have an extra branch,
>> and the write might serve to prefetch the cache line.
>>
>> > Writes to the used event field (vring_used_event) are still unconditional.
>> >
>> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>
>> Wow you are right wrt the spec. Should we change the spec or the
>> code though?
>
> I would change the spec and note that bit 0 of the flags is ignored if
> event indices are in use.

Changing the spec sounds good. I'll see if I can get any meaningful
perf numbers with vring_bench, just in case. Would there be any
interest in having the tool checked in the tree? There are several
commits referencing vring_bench but it seems to exist only in a list
archive - took me a while to figure it out.

Also apologies for posting in a wrong list.

Thanks,
Ladi

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Ladi Prosek]

On Mon, Jun 6, 2016 at 11:31 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> On Mon, Jun 6, 2016 at 4:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 06/06/2016 15:55, Michael S. Tsirkin wrote:
>>> > According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
>>> > negotiated the driver MUST set flags to 0. Not dirtying the available
>>> > ring in virtqueue_disable_cb may also have a positive performance impact.
>>>
>>> Question would be, is this a gain or a loss. We have an extra branch,
>>> and the write might serve to prefetch the cache line.
>>>
>>> > Writes to the used event field (vring_used_event) are still unconditional.
>>> >
>>> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>>
>>> Wow you are right wrt the spec. Should we change the spec or the
>>> code though?
>>
>> I would change the spec and note that bit 0 of the flags is ignored if
>> event indices are in use.
>
> Changing the spec sounds good. I'll see if I can get any meaningful
> perf numbers with vring_bench, just in case. Would there be any
> interest in having the tool checked in the tree? There are several
> commits referencing vring_bench but it seems to exist only in a list
> archive - took me a while to figure it out.

vring_bench with two threads, host thread polls the queue and moves
indices from available to used, guest thread polls returned indices
with:

do {
  virtqueue_disable_cb(vq);
  while ((p = virtqueue_get_buf(vq, &len)) != NULL)
    returned++;
  if (unlikely(virtqueue_is_broken(vq)))
    break;
} while (!virtqueue_enable_cb(vq));

Results:
- no effect on branch misses
- L1 dcache load misses ~0.5% down but with ~0.5% variance so not
super convincing

Ladi

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Michael S. Tsirkin]

On Mon, Jun 06, 2016 at 11:51:34AM +0200, Ladi Prosek wrote:
> According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
> negotiated the driver MUST set flags to 0. Not dirtying the available
> ring in virtqueue_disable_cb may also have a positive performance impact.
> 
> Writes to the used event field (vring_used_event) are still unconditional.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>

Write it to match Linux coding style please:
	if (!vq->event)
		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);

Also, please Cc stable as this is a spec compliance issue.

Looks like older Linux guests will have to cherry-pick
    virtio_ring: shadow available ring flags & index
to fix this. Pls mention this when you resubmit.

And pls Cc virtio lists as per MAINTAINERS.

We should also list this as one of the differences between
virtio 1.0 and 0.9.X.

Also Cc windows driver devs to make sure windows drivers do not
have spec issues.


> ---
>  drivers/virtio/virtio_ring.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index ca6bfdd..d6345e1 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -718,7 +718,9 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
>  
>  	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
>  		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  
>  }
> @@ -750,7 +752,9 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>  	 * entry. Always do both to keep code simple. */
>  	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>  		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
>  	END_USE(vq);
> @@ -818,10 +822,12 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>  	 * more to do. */
>  	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
>  	 * either clear the flags bit or point the event index at the next
> -	 * entry. Always do both to keep code simple. */
> +	 * entry. Always update the event index to keep code simple. */
>  	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>  		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  	/* TODO: tune this threshold */
>  	bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
> @@ -939,7 +945,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>  	/* No callback?  Tell other side not to bother us. */
>  	if (!callback) {
>  		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  
>  	/* Put everything in free lists. */
> -- 
> 2.5.5

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Michael S. Tsirkin]

On Wed, Jun 08, 2016 at 02:58:04PM +0200, Ladi Prosek wrote:
> On Mon, Jun 6, 2016 at 11:31 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> > On Mon, Jun 6, 2016 at 4:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >>
> >> On 06/06/2016 15:55, Michael S. Tsirkin wrote:
> >>> > According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
> >>> > negotiated the driver MUST set flags to 0. Not dirtying the available
> >>> > ring in virtqueue_disable_cb may also have a positive performance impact.
> >>>
> >>> Question would be, is this a gain or a loss. We have an extra branch,
> >>> and the write might serve to prefetch the cache line.
> >>>
> >>> > Writes to the used event field (vring_used_event) are still unconditional.
> >>> >
> >>> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >>>
> >>> Wow you are right wrt the spec. Should we change the spec or the
> >>> code though?
> >>
> >> I would change the spec and note that bit 0 of the flags is ignored if
> >> event indices are in use.
> >
> > Changing the spec sounds good. I'll see if I can get any meaningful
> > perf numbers with vring_bench, just in case. Would there be any
> > interest in having the tool checked in the tree? There are several
> > commits referencing vring_bench but it seems to exist only in a list
> > archive - took me a while to figure it out.
> 
> vring_bench with two threads, host thread polls the queue and moves
> indices from available to used, guest thread polls returned indices
> with:
> 
> do {
>   virtqueue_disable_cb(vq);
>   while ((p = virtqueue_get_buf(vq, &len)) != NULL)
>     returned++;
>   if (unlikely(virtqueue_is_broken(vq)))
>     break;
> } while (!virtqueue_enable_cb(vq));
> 
> Results:
> - no effect on branch misses
> - L1 dcache load misses ~0.5% down but with ~0.5% variance so not
> super convincing
> 
> Ladi

I think it makes sense to change this - the reason it
was written like this is because we did not have
a shadow, it was easier to change and check the flags directly.

Did you open an issue with virtio spec?

-- 
MST

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Ladi Prosek]

On Mon, Aug 22, 2016 at 4:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Jun 06, 2016 at 11:51:34AM +0200, Ladi Prosek wrote:
>> According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
>> negotiated the driver MUST set flags to 0. Not dirtying the available
>> ring in virtqueue_disable_cb may also have a positive performance impact.
>>
>> Writes to the used event field (vring_used_event) are still unconditional.
>>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>
> Write it to match Linux coding style please:
>         if (!vq->event)
>                 vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
>
> Also, please Cc stable as this is a spec compliance issue.
>
> Looks like older Linux guests will have to cherry-pick
>     virtio_ring: shadow available ring flags & index
> to fix this. Pls mention this when you resubmit.
>
> And pls Cc virtio lists as per MAINTAINERS.

Will do.

> We should also list this as one of the differences between
> virtio 1.0 and 0.9.X.
>
> Also Cc windows driver devs to make sure windows drivers do not
> have spec issues.

Windows is pretty much identical to Linux when it comes to ring routines.
    virtio_ring: shadow available ring flags & index
has already been ported to virtio-win and I'll do the same for this
patch if it gets merged.

>> ---
>>  drivers/virtio/virtio_ring.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index ca6bfdd..d6345e1 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -718,7 +718,9 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
>>
>>       if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
>>               vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
>> -             vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
>> +             if (!vq->event) {
>> +                     vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
>> +             }
>>       }
>>
>>  }
>> @@ -750,7 +752,9 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>>        * entry. Always do both to keep code simple. */
>>       if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>>               vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
>> -             vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
>> +             if (!vq->event) {
>> +                     vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
>> +             }
>>       }
>>       vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
>>       END_USE(vq);
>> @@ -818,10 +822,12 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>>        * more to do. */
>>       /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
>>        * either clear the flags bit or point the event index at the next
>> -      * entry. Always do both to keep code simple. */
>> +      * entry. Always update the event index to keep code simple. */
>>       if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>>               vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
>> -             vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
>> +             if (!vq->event) {
>> +                     vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
>> +             }
>>       }
>>       /* TODO: tune this threshold */
>>       bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
>> @@ -939,7 +945,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>>       /* No callback?  Tell other side not to bother us. */
>>       if (!callback) {
>>               vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
>> -             vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
>> +             if (!vq->event) {
>> +                     vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
>> +             }
>>       }
>>
>>       /* Put everything in free lists. */
>> --
>> 2.5.5

Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

[Ladi Prosek]

On Mon, Aug 22, 2016 at 4:29 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jun 08, 2016 at 02:58:04PM +0200, Ladi Prosek wrote:
>> On Mon, Jun 6, 2016 at 11:31 PM, Ladi Prosek <lprosek@redhat.com> wrote:
>> > On Mon, Jun 6, 2016 at 4:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >>
>> >>
>> >> On 06/06/2016 15:55, Michael S. Tsirkin wrote:
>> >>> > According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
>> >>> > negotiated the driver MUST set flags to 0. Not dirtying the available
>> >>> > ring in virtqueue_disable_cb may also have a positive performance impact.
>> >>>
>> >>> Question would be, is this a gain or a loss. We have an extra branch,
>> >>> and the write might serve to prefetch the cache line.
>> >>>
>> >>> > Writes to the used event field (vring_used_event) are still unconditional.
>> >>> >
>> >>> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> >>>
>> >>> Wow you are right wrt the spec. Should we change the spec or the
>> >>> code though?
>> >>
>> >> I would change the spec and note that bit 0 of the flags is ignored if
>> >> event indices are in use.
>> >
>> > Changing the spec sounds good. I'll see if I can get any meaningful
>> > perf numbers with vring_bench, just in case. Would there be any
>> > interest in having the tool checked in the tree? There are several
>> > commits referencing vring_bench but it seems to exist only in a list
>> > archive - took me a while to figure it out.
>>
>> vring_bench with two threads, host thread polls the queue and moves
>> indices from available to used, guest thread polls returned indices
>> with:
>>
>> do {
>>   virtqueue_disable_cb(vq);
>>   while ((p = virtqueue_get_buf(vq, &len)) != NULL)
>>     returned++;
>>   if (unlikely(virtqueue_is_broken(vq)))
>>     break;
>> } while (!virtqueue_enable_cb(vq));
>>
>> Results:
>> - no effect on branch misses
>> - L1 dcache load misses ~0.5% down but with ~0.5% variance so not
>> super convincing
>>
>> Ladi
>
> I think it makes sense to change this - the reason it
> was written like this is because we did not have
> a shadow, it was easier to change and check the flags directly.
>
> Did you open an issue with virtio spec?

I did not. Do you want me to?

> --
> MST