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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxxxxxxx
Thanks!
> 2.5.5
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
