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

2022-08-04 Thread Jason Wang 
On Thu, Aug 4, 2022 at 2:21 PM Eugenio Perez Martin  wrote:
>
> On Thu, Aug 4, 2022 at 5:14 AM Jason Wang  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 
> > > ---
> > >   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;
> > > +}
> > >   }
> > >
> > >   /**
> >
>

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

2022-08-03 Thread Eugenio Perez Martin 
On Thu, Aug 4, 2022 at 5:14 AM Jason Wang  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 
> > ---
> >   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

2022-08-03 Thread Jason Wang 



在 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 
---
  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;
+}
  }
  
  /**

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

2022-08-02 Thread 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 
---
 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