[PATCH v5 03/10] vhost: Do not depend on !NULL VirtQueueElement on vhost_svq_flush

Eugenio Pérez posted 10 patches 3 years, 6 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>
There is a newer version of this series
[PATCH v5 03/10] vhost: Do not depend on !NULL VirtQueueElement on vhost_svq_flush
Posted by Eugenio Pérez 3 years, 6 months ago
Since QEMU will be able to inject new elements on CVQ to restore the
state, we need not to depend on a VirtQueueElement to know if a new
element has been used by the device or not. Instead of check that, check
if there are new elements only using used idx on vhost_svq_flush.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index e6eebd0e8d..fdb550c31b 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -491,7 +491,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
 /**
  * Poll the SVQ for one device used buffer.
  *
- * This function race with main event loop SVQ polling, so extra
+ * This function races with main event loop SVQ polling, so extra
  * synchronization is needed.
  *
  * Return the length written by the device.
@@ -499,20 +499,20 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
 size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
 {
     int64_t start_us = g_get_monotonic_time();
-    do {
+    while (true) {
         uint32_t len;
-        VirtQueueElement *elem = vhost_svq_get_buf(svq, &len);
-        if (elem) {
-            return len;
-        }
 
         if (unlikely(g_get_monotonic_time() - start_us > 10e6)) {
             return 0;
         }
 
-        /* Make sure we read new used_idx */
-        smp_rmb();
-    } while (true);
+        if (!vhost_svq_more_used(svq)) {
+            continue;
+        }
+
+        vhost_svq_get_buf(svq, &len);
+        return len;
+    }
 }
 
 /**
-- 
2.31.1


Re: [PATCH v5 03/10] vhost: Do not depend on !NULL VirtQueueElement on vhost_svq_flush
Posted by Jason Wang 3 years, 6 months ago
在 2022/8/3 01:57, Eugenio Pérez 写道:
> Since QEMU will be able to inject new elements on CVQ to restore the
> state, we need not to depend on a VirtQueueElement to know if a new
> element has been used by the device or not. Instead of check that, check
> if there are new elements only using used idx on vhost_svq_flush.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index e6eebd0e8d..fdb550c31b 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -491,7 +491,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>   /**
>    * Poll the SVQ for one device used buffer.
>    *
> - * This function race with main event loop SVQ polling, so extra
> + * This function races with main event loop SVQ polling, so extra
>    * synchronization is needed.
>    *
>    * Return the length written by the device.
> @@ -499,20 +499,20 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>   size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
>   {
>       int64_t start_us = g_get_monotonic_time();
> -    do {
> +    while (true) {
>           uint32_t len;
> -        VirtQueueElement *elem = vhost_svq_get_buf(svq, &len);
> -        if (elem) {
> -            return len;
> -        }
>   
>           if (unlikely(g_get_monotonic_time() - start_us > 10e6)) {
>               return 0;
>           }
>   
> -        /* Make sure we read new used_idx */
> -        smp_rmb();
> -    } while (true);
> +        if (!vhost_svq_more_used(svq)) {
> +            continue;
> +        }
> +
> +        vhost_svq_get_buf(svq, &len);


I wonder if this means we won't worry about the infinite wait?

Thanks


> +        return len;
> +    }
>   }
>   
>   /**


Re: [PATCH v5 03/10] vhost: Do not depend on !NULL VirtQueueElement on vhost_svq_flush
Posted by Eugenio Perez Martin 3 years, 6 months ago
On Thu, Aug 4, 2022 at 5:14 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/8/3 01:57, Eugenio Pérez 写道:
> > Since QEMU will be able to inject new elements on CVQ to restore the
> > state, we need not to depend on a VirtQueueElement to know if a new
> > element has been used by the device or not. Instead of check that, check
> > if there are new elements only using used idx on vhost_svq_flush.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index e6eebd0e8d..fdb550c31b 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -491,7 +491,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >   /**
> >    * Poll the SVQ for one device used buffer.
> >    *
> > - * This function race with main event loop SVQ polling, so extra
> > + * This function races with main event loop SVQ polling, so extra
> >    * synchronization is needed.
> >    *
> >    * Return the length written by the device.
> > @@ -499,20 +499,20 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >   size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
> >   {
> >       int64_t start_us = g_get_monotonic_time();
> > -    do {
> > +    while (true) {
> >           uint32_t len;
> > -        VirtQueueElement *elem = vhost_svq_get_buf(svq, &len);
> > -        if (elem) {
> > -            return len;
> > -        }
> >
> >           if (unlikely(g_get_monotonic_time() - start_us > 10e6)) {
> >               return 0;
> >           }
> >
> > -        /* Make sure we read new used_idx */
> > -        smp_rmb();
> > -    } while (true);
> > +        if (!vhost_svq_more_used(svq)) {
> > +            continue;
> > +        }
> > +
> > +        vhost_svq_get_buf(svq, &len);
>
>
> I wonder if this means we won't worry about the infinite wait?
>

vhost_svq_get_buf call doesn't block, and the check for the timeout is
immediately above the check for new descriptors. Am I missing
something?

> Thanks
>
>
> > +        return len;
> > +    }
> >   }
> >
> >   /**
>
Re: [PATCH v5 03/10] vhost: Do not depend on !NULL VirtQueueElement on vhost_svq_flush
Posted by Jason Wang 3 years, 6 months ago
On Thu, Aug 4, 2022 at 2:21 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Thu, Aug 4, 2022 at 5:14 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/8/3 01:57, Eugenio Pérez 写道:
> > > Since QEMU will be able to inject new elements on CVQ to restore the
> > > state, we need not to depend on a VirtQueueElement to know if a new
> > > element has been used by the device or not. Instead of check that, check
> > > if there are new elements only using used idx on vhost_svq_flush.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++---------
> > >   1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > index e6eebd0e8d..fdb550c31b 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -491,7 +491,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> > >   /**
> > >    * Poll the SVQ for one device used buffer.
> > >    *
> > > - * This function race with main event loop SVQ polling, so extra
> > > + * This function races with main event loop SVQ polling, so extra
> > >    * synchronization is needed.
> > >    *
> > >    * Return the length written by the device.
> > > @@ -499,20 +499,20 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> > >   size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
> > >   {
> > >       int64_t start_us = g_get_monotonic_time();
> > > -    do {
> > > +    while (true) {
> > >           uint32_t len;
> > > -        VirtQueueElement *elem = vhost_svq_get_buf(svq, &len);
> > > -        if (elem) {
> > > -            return len;
> > > -        }
> > >
> > >           if (unlikely(g_get_monotonic_time() - start_us > 10e6)) {
> > >               return 0;
> > >           }
> > >
> > > -        /* Make sure we read new used_idx */
> > > -        smp_rmb();
> > > -    } while (true);
> > > +        if (!vhost_svq_more_used(svq)) {
> > > +            continue;
> > > +        }
> > > +
> > > +        vhost_svq_get_buf(svq, &len);
> >
> >
> > I wonder if this means we won't worry about the infinite wait?
> >
>
> vhost_svq_get_buf call doesn't block, and the check for the timeout is
> immediately above the check for new descriptors. Am I missing
> something?

No, I misread the code.

Sorry.

Thanks

>
> > Thanks
> >
> >
> > > +        return len;
> > > +    }
> > >   }
> > >
> > >   /**
> >
>