From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant Date: Mon, 6 Jun 2016 16:55:16 +0300 Message-ID: <20160606165015-mutt-send-email-mst@redhat.com> References: <1465206694-1150-1-git-send-email-lprosek@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Ladi Prosek Return-path: Received: from mx1.redhat.com ([209.132.183.28]:53445 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751925AbcFFNzT (ORCPT ); Mon, 6 Jun 2016 09:55:19 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0E9E781129 for ; Mon, 6 Jun 2016 13:55:19 +0000 (UTC) Content-Disposition: inline In-Reply-To: <1465206694-1150-1-git-send-email-lprosek@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 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