Re: [RFC PATCH v9 04/23] vhost: Get vring base from vq, not svq

[Jason Wang]

在 2022/7/8 18:10, Eugenio Perez Martin 写道:
> On Fri, Jul 8, 2022 at 11:12 AM Jason Wang <jasowang@redhat.com> wrote:
> > On Thu, Jul 7, 2022 at 2:40 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > The used idx used to match with this, but it will not match from the
> > > moment we introduce svq_inject.
> > It might be better to explain what "svq_inject" means here.
> Good point, I'll change for the next version.
>
> > > Rewind all the descriptors not used by
> > > vdpa device and get the vq state properly.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   include/hw/virtio/virtio.h | 1 +
> > >   hw/virtio/vhost-vdpa.c     | 7 +++----
> > >   hw/virtio/virtio.c         | 5 +++++
> > >   3 files changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > index db1c0ddf6b..4b51ab9d06 100644
> > > --- a/include/hw/virtio/virtio.h
> > > +++ b/include/hw/virtio/virtio.h
> > > @@ -302,6 +302,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int 
> > > n);
> > >   hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n);
> > >   hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n);
> > >   unsigned int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n);
> > > +unsigned int virtio_queue_get_in_use(const VirtQueue *vq);
> > >   void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n,
> > >                                        unsigned int idx);
> > >   void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n);
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index 2ee8009594..de76128030 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -1189,12 +1189,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev 
> > > *dev,
> > >                                          struct vhost_vring_state *ring)
> > >   {
> > >       struct vhost_vdpa *v = dev->opaque;
> > > -    int vdpa_idx = ring->index - dev->vq_index;
> > >       int ret;
> > >
> > >       if (v->shadow_vqs_enabled) {
> > > -        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx);
> > > -
> > > +        const VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
> > >           /*
> > >            * Setting base as last used idx, so destination will see as available
> > >            * all the entries that the device did not use, including the 
> > > in-flight
> > > @@ -1203,7 +1201,8 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev 
> > > *dev,
> > >            * TODO: This is ok for networking, but other kinds of devices might
> > >            * have problems with these retransmissions.
> > >            */
> > > -        ring->num = svq->last_used_idx;
> > > +        ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index) -
> > > +                    virtio_queue_get_in_use(vq);
> > I think we need to change the above comment as well otherwise readers
> > might get confused.
> Re-thinking this: This part has always been buggy, so this is actually
> a fix. I'll tag it for next versions or, even better, send it
> separately.
>
> But the comment still holds: We cannot use the device's used idx since
> it could not match with the guest visible one. This is actually easy
> to trigger if we migrate a guest many times with traffic.


I may miss someting, maybe you can give me an example on this (I assume 
the size of the svq is the same as what guest can see).


> Maybe it's cleaner to export directly used_idx from VirtQueue? Extra
> care is needed with packed vq, but SVQ still does not support it. I
> didn't want to duplicate that logic in virtio ring handling.


So two more questions here:

1) what's the reason of rewinding via virtio_queue_get_in_use()?

2) it looks like we could end up with underflow with the above math?

Thanks


> > I wonder why we need to bother at this time. Is this an issue for
> > networking devices?
> Every device has this issue when migrating as soon as the device's
> used index is not the same as the guest's one.
>
> > And for block device, it's not sufficient since
> > there's no guarantee that the descriptor is handled in order?
> Right, that part still hold here.
>
> Thanks!