On Fri, Jul 15, 2022 at 1:39 PM Eugenio Perez Martin
<epere...@redhat.com> wrote:
>
> On Fri, Jul 15, 2022 at 5:59 AM Jason Wang <jasow...@redhat.com> wrote:
> >
> > On Fri, Jul 15, 2022 at 12:32 AM Eugenio Pérez <epere...@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 <epere...@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.
What do you mean by "that check" here?
>
> 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?
I'm not sure I get the goal of the smp_rmb() here. What barrier does it pair?
Since we are in the busy loop, we will read the for new used_idx for
sure, and we can't forecast when the used_idx is committed to memory.
Thanks
>
> Thanks!
>
>
> > Thanks
> >
> > > + } while (true);
> > > +}
> > > +
> > > /**
> > > * Forward used buffers.
> > > *
> > > --
> > > 2.31.1
> > >
> >
>
