virtio: signal after wrapping packed used_idx

[PATCH] virtio: signal after wrapping packed used_idx

Posted by Stefan Hajnoczi 2 years, 4 months ago

Packed Virtqueues wrap used_idx instead of letting it run freely like
Split Virtqueues do. If the used ring wraps more than once there is no
way to compare vq->signalled_used and vq->used_idx in
virtio_packed_should_notify() since they are modulo vq->vring.num.

This causes the device to stop sending used buffer notifications when
when virtio_packed_should_notify() is called less than once each time
around the used ring.

It is possible to trigger this with virtio-blk's dataplane
notify_guest_bh() irq coalescing optimization. The call to
virtio_notify_irqfd() (and virtio_packed_should_notify()) is deferred to
a BH. If the guest driver is polling it can complete and submit more
requests before the BH executes, causing the used ring to wrap more than
once. The result is that the virtio-blk device ceases to raise
interrupts and I/O hangs.

Cc: Tiwei Bie <tiwei.bie@intel.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Smarter solutions welcome, but I think notifying once per vq->vring.num
is acceptable.
---
 hw/virtio/virtio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ea7c079fb0..f7851c2750 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -885,6 +885,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
     if (vq->used_idx >= vq->vring.num) {
         vq->used_idx -= vq->vring.num;
         vq->used_wrap_counter ^= 1;
+        vq->signalled_used_valid = false;
     }
 }
 
-- 
2.33.1

Re: [PATCH] virtio: signal after wrapping packed used_idx

Posted by Michael S. Tsirkin 2 years, 4 months ago

On Tue, Nov 30, 2021 at 01:45:10PM +0000, Stefan Hajnoczi wrote:
> Packed Virtqueues wrap used_idx instead of letting it run freely like
> Split Virtqueues do. If the used ring wraps more than once there is no
> way to compare vq->signalled_used and vq->used_idx in
> virtio_packed_should_notify() since they are modulo vq->vring.num.
> 
> This causes the device to stop sending used buffer notifications when
> when virtio_packed_should_notify() is called less than once each time
> around the used ring.
> 
> It is possible to trigger this with virtio-blk's dataplane
> notify_guest_bh() irq coalescing optimization. The call to
> virtio_notify_irqfd() (and virtio_packed_should_notify()) is deferred to
> a BH. If the guest driver is polling it can complete and submit more
> requests before the BH executes, causing the used ring to wrap more than
> once. The result is that the virtio-blk device ceases to raise
> interrupts and I/O hangs.
> 
> Cc: Tiwei Bie <tiwei.bie@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Makes sense.  Fixes tag?

> ---
> Smarter solutions welcome, but I think notifying once per vq->vring.num
> is acceptable.
> ---
>  hw/virtio/virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ea7c079fb0..f7851c2750 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -885,6 +885,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>      if (vq->used_idx >= vq->vring.num) {
>          vq->used_idx -= vq->vring.num;
>          vq->used_wrap_counter ^= 1;
> +        vq->signalled_used_valid = false;
>      }
>  }
>  
> -- 
> 2.33.1

Re: [PATCH] virtio: signal after wrapping packed used_idx

Posted by Stefan Hajnoczi 2 years, 4 months ago

On Tue, Nov 30, 2021 at 06:40:49PM -0500, Michael S. Tsirkin wrote:
> On Tue, Nov 30, 2021 at 01:45:10PM +0000, Stefan Hajnoczi wrote:
> > Packed Virtqueues wrap used_idx instead of letting it run freely like
> > Split Virtqueues do. If the used ring wraps more than once there is no
> > way to compare vq->signalled_used and vq->used_idx in
> > virtio_packed_should_notify() since they are modulo vq->vring.num.
> > 
> > This causes the device to stop sending used buffer notifications when
> > when virtio_packed_should_notify() is called less than once each time
> > around the used ring.
> > 
> > It is possible to trigger this with virtio-blk's dataplane
> > notify_guest_bh() irq coalescing optimization. The call to
> > virtio_notify_irqfd() (and virtio_packed_should_notify()) is deferred to
> > a BH. If the guest driver is polling it can complete and submit more
> > requests before the BH executes, causing the used ring to wrap more than
> > once. The result is that the virtio-blk device ceases to raise
> > interrupts and I/O hangs.
> > 
> > Cc: Tiwei Bie <tiwei.bie@intel.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Makes sense.  Fixes tag?

Good idea.

Fixes: 86044b24e865fb9596ed77a4d0f3af8b90a088a1 ("virtio: basic packed virtqueue support")

Re: [PATCH] virtio: signal after wrapping packed used_idx

Posted by Jason Wang 2 years, 4 months ago

On Tue, Nov 30, 2021 at 9:46 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Packed Virtqueues wrap used_idx instead of letting it run freely like
> Split Virtqueues do. If the used ring wraps more than once there is no
> way to compare vq->signalled_used and vq->used_idx in
> virtio_packed_should_notify() since they are modulo vq->vring.num.
>
> This causes the device to stop sending used buffer notifications when
> when virtio_packed_should_notify() is called less than once each time
> around the used ring.
>
> It is possible to trigger this with virtio-blk's dataplane
> notify_guest_bh() irq coalescing optimization. The call to
> virtio_notify_irqfd() (and virtio_packed_should_notify()) is deferred to
> a BH. If the guest driver is polling it can complete and submit more
> requests before the BH executes, causing the used ring to wrap more than
> once. The result is that the virtio-blk device ceases to raise
> interrupts and I/O hangs.
>
> Cc: Tiwei Bie <tiwei.bie@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

> ---
> Smarter solutions welcome, but I think notifying once per vq->vring.num
> is acceptable.
> ---
>  hw/virtio/virtio.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ea7c079fb0..f7851c2750 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -885,6 +885,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>      if (vq->used_idx >= vq->vring.num) {
>          vq->used_idx -= vq->vring.num;
>          vq->used_wrap_counter ^= 1;
> +        vq->signalled_used_valid = false;
>      }
>  }
>
> --
> 2.33.1
>