When we check if a driver needs a signal, we compare:

- used_event: written by the driver each time it consumes an item
- new: current idx written to the used ring, updated by us
- old: last idx we signaled about

We call vring_need_event() which does:

    return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old);

Previously we updated signalled_used on every check, so old was always
new - 1. Because used_event cannot bigger than new_idx, this check
becomes (ignoring wrapping):

    return new_idx == event_idx + 1;

Since the driver consumes items at the same time the device produces
items, it is very likely (and seen in logs) that the driver used_event
is too far behind new_idx and we don't signal the driver.

With libblkio virtio-blk-vhost-user driver, if the driver does not get a
signal, the libblkio client can hang polling the completion fd. This
is very easy to reproduce on some machines and impossible to reproduce
on others.

Fixed by updating signalled_used only when we signal the driver.
Tested using blkio-bench and libblkio client application that used to
hang randomly without this change.

Buglink: https://gitlab.com/libblkio/libblkio/-/issues/68
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 8fb61e2df2..5f26d2d378 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -2382,12 +2382,11 @@ vu_queue_empty(VuDev *dev, VuVirtq *vq)
 }
 
 static bool
 vring_notify(VuDev *dev, VuVirtq *vq)
 {
-    uint16_t old, new;
-    bool v;
+    uint16_t old, new, used;
 
     /* We need to expose used array entries before checking used event. */
     smp_mb();
 
     /* Always notify when queue is empty (when feature acknowledge) */
@@ -2398,15 +2397,27 @@ vring_notify(VuDev *dev, VuVirtq *vq)
 
     if (!vu_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
         return !(vring_avail_flags(vq) & VRING_AVAIL_F_NO_INTERRUPT);
     }
 
-    v = vq->signalled_used_valid;
-    vq->signalled_used_valid = true;
+    if (!vq->signalled_used_valid) {
+        vq->signalled_used_valid = true;
+        vq->signalled_used = vq->used_idx;
+        return true;
+    }
+
+    used = vring_get_used_event(vq);
+    new = vq->used_idx;
     old = vq->signalled_used;
-    new = vq->signalled_used = vq->used_idx;
-    return !v || vring_need_event(vring_get_used_event(vq), new, old);
+
+    if (vring_need_event(used, new, old)) {
+        vq->signalled_used_valid = true;
+        vq->signalled_used = vq->used_idx;
+        return true;
+    }
+
+    return false;
 }
 
 static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
 {
     if (unlikely(dev->broken) ||
-- 
2.40.1

On Tue, 9 May 2023 at 06:28, Nir Soffer <nsoffer@redhat.com> wrote:
>
> When we check if a driver needs a signal, we compare:
>
> - used_event: written by the driver each time it consumes an item

In practice drivers tend to update it as you described, but devices
cannot make that assumption.

used_event is simply the index at which the driver wishes to receive
the next used buffer notification. It does not need to be updated each
time a used buffer is consumed.

> - new: current idx written to the used ring, updated by us
> - old: last idx we signaled about
>
> We call vring_need_event() which does:
>
>     return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old);
>
> Previously we updated signalled_used on every check, so old was always
> new - 1.

libvhost-user leaves it up to the application to decide when to call
vu_queue_notify(). It might be called once after pushing multiple used
buffers. We cannot assume it is always new - 1.

> Because used_event cannot bigger than new_idx, this check
> becomes (ignoring wrapping):
>
>     return new_idx == event_idx + 1;

This is not true. used_event is an arbitrary index value that can be
larger than new_idx. For example, if the driver wants to be notified
after 5 completions, it will set it to a future value that is greater
than new_idx.

> Since the driver consumes items at the same time the device produces
> items, it is very likely (and seen in logs) that the driver used_event
> is too far behind new_idx and we don't signal the driver.

This is expected and must be handled by the driver. Here is the Rust
virtio-driver code:

impl<'a, T: Clone> VirtqueueRing<'a, T> {
...
    fn pop(&mut self) -> Option<T> {
        if self.has_next() {
            let result = unsafe { (*self.ptr).ring[self.ring_idx()].clone() };
            self.next_idx += Wrapping(1);
            Some(result)
        } else {
            None
        }
    }
...
}

impl<'a, 'queue, R: Copy> Iterator for VirtqueueIter<'a, 'queue, R> {
    type Item = VirtqueueCompletion<R>;

    fn next(&mut self) -> Option<Self::Item> {
        if let Some(next) = self.virtqueue.used.pop() {
            let idx = (next.idx.to_native() %
(self.virtqueue.queue_size as u32)) as u16;
            self.virtqueue.free_desc(idx);
            if self.virtqueue.event_idx_enabled &&
self.virtqueue.used_notif_enabled {
                self.virtqueue.update_used_event();
            }

            let req = unsafe { *self.virtqueue.req.offset(idx as isize) };
            Some(VirtqueueCompletion { idx, req })
        } else {
            if self.virtqueue.event_idx_enabled &&
!self.virtqueue.used_notif_enabled {
                self.virtqueue.update_used_event();
            }
            None
        }
    }

When the caller uses VirtqueueIter to iterate over completed requests,
the race is automatically handled:

1. If the device sees the updated used_event value, then a Used Buffer
   Notification is sent. No notification is missed.

2. If the device sees the outdated used_event value, then no Used Buffer
   Notification is sent. Keep in mind that the device placed the Used
   Buffer into the Used Ring *before* checking used_event. The driver
   updates used_event before calling VirtqueueIter.next() again. Therefore,
   VirtqueueIter.next() is guaranteed to see the Used Buffer although no
   Used Buffer Notification was sent for it.

There is no scenario where VirtqueueIter does not see the new Used
Buffer and there is no Used Buffer Notification.

However, this relies on the driver always fully iterating - if it stops early
then notifications might be dropped.

> With libblkio virtio-blk-vhost-user driver, if the driver does not get a
> signal, the libblkio client can hang polling the completion fd. This
> is very easy to reproduce on some machines and impossible to reproduce
> on others.
>
> Fixed by updating signalled_used only when we signal the driver.
> Tested using blkio-bench and libblkio client application that used to
> hang randomly without this change.

QEMU works fine with the same code as libvhost-user though:

static bool virtio_split_should_notify(VirtIODevice *vdev, VirtQueue *vq)
{
    uint16_t old, new;
    bool v;
    /* We need to expose used array entries before checking used event. */
    smp_mb();
    /* Always notify when queue is empty (when feature acknowledge) */
    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
        !vq->inuse && virtio_queue_empty(vq)) {
        return true;
    }

    if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
        return !(vring_avail_flags(vq) & VRING_AVAIL_F_NO_INTERRUPT);
    }

    v = vq->signalled_used_valid;
    vq->signalled_used_valid = true;
    old = vq->signalled_used;
    new = vq->signalled_used = vq->used_idx;
    return !v || vring_need_event(vring_get_used_event(vq), new, old);
}

I suspect there is a bug in the libblkio or virtio-driver code.
Unfortunately I've looked at the code a few times and couldn't spot
the issue :(.

>
> Buglink: https://gitlab.com/libblkio/libblkio/-/issues/68
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 8fb61e2df2..5f26d2d378 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -2382,12 +2382,11 @@ vu_queue_empty(VuDev *dev, VuVirtq *vq)
>  }
>
>  static bool
>  vring_notify(VuDev *dev, VuVirtq *vq)
>  {
> -    uint16_t old, new;
> -    bool v;
> +    uint16_t old, new, used;
>
>      /* We need to expose used array entries before checking used event. */
>      smp_mb();
>
>      /* Always notify when queue is empty (when feature acknowledge) */
> @@ -2398,15 +2397,27 @@ vring_notify(VuDev *dev, VuVirtq *vq)
>
>      if (!vu_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
>          return !(vring_avail_flags(vq) & VRING_AVAIL_F_NO_INTERRUPT);
>      }
>
> -    v = vq->signalled_used_valid;
> -    vq->signalled_used_valid = true;
> +    if (!vq->signalled_used_valid) {
> +        vq->signalled_used_valid = true;
> +        vq->signalled_used = vq->used_idx;
> +        return true;
> +    }
> +
> +    used = vring_get_used_event(vq);
> +    new = vq->used_idx;
>      old = vq->signalled_used;
> -    new = vq->signalled_used = vq->used_idx;
> -    return !v || vring_need_event(vring_get_used_event(vq), new, old);
> +
> +    if (vring_need_event(used, new, old)) {
> +        vq->signalled_used_valid = true;
> +        vq->signalled_used = vq->used_idx;
> +        return true;
> +    }
> +
> +    return false;
>  }
>
>  static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
>  {
>      if (unlikely(dev->broken) ||
> --
> 2.40.1
>
>