| Message ID | 1465206694-1150-1-git-send-email-lprosek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. */
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(-)