Re: [PATCH v2 12/19] vhost: add vhost_svq_poll

[Eugenio Perez Martin]

On Fri, Jul 15, 2022 at 5:59 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jul 15, 2022 at 12:32 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > It allows the Shadow Control VirtQueue to wait for the device to use the
> > available buffers.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-shadow-virtqueue.h |  1 +
> >  hw/virtio/vhost-shadow-virtqueue.c | 22 ++++++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> > b/hw/virtio/vhost-shadow-virtqueue.h
> > index 1692541cbb..b5c6e3b3b4 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -89,6 +89,7 @@ void vhost_svq_push_elem(VhostShadowVirtqueue *svq, const 
> > SVQElement *elem,
> >  int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
> >                    size_t out_num, const struct iovec *in_sg, size_t in_num,
> >                    SVQElement *elem);
> > +size_t vhost_svq_poll(VhostShadowVirtqueue *svq);
> >
> >  void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
> >  void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq, int call_fd);
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index 5244896358..31a267f721 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -486,6 +486,28 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >      } while (!vhost_svq_enable_notification(svq));
> >  }
> >
> > +/**
> > + * Poll the SVQ for one device used buffer.
> > + *
> > + * This function race with main event loop SVQ polling, so extra
> > + * synchronization is needed.
> > + *
> > + * Return the length written by the device.
> > + */
> > +size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
> > +{
> > +    do {
> > +        uint32_t len;
> > +        SVQElement *elem = vhost_svq_get_buf(svq, &len);
> > +        if (elem) {
> > +            return len;
> > +        }
> > +
> > +        /* Make sure we read new used_idx */
> > +        smp_rmb();
>
> There's already one smp_rmb(0 in vhost_svq_get_buf(). So this seems useless?
>

That rmb is after checking for new entries with (vq->last_used_idx !=
svq->shadow_used_idx) , to avoid reordering used_idx read with the
actual used entry. So my understanding is
that the compiler is free to skip that check within the while loop.

Maybe the right solution is to add it in vhost_svq_more_used after the
condition (vq->last_used_idx != svq->shadow_used_idx) is false?

Thanks!


> Thanks
>
> > +    } while (true);
> > +}
> > +
> >  /**
> >   * Forward used buffers.
> >   *
> > --
> > 2.31.1
> >
>