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

["Michael S. Tsirkin" <mst@redhat.com>]

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