Re: [PATCH 11/16] vhost: introduce restart and release for vhost_dev's vqs

[Kangjie Xu]

在 2022/7/27 12:47, Jason Wang 写道:
> On Tue, Jul 26, 2022 at 1:13 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
> > 在 2022/7/26 12:13, Jason Wang 写道:
> > > 在 2022/7/18 19:17, Kangjie Xu 写道:
> > > > Introduce vhost_dev_virtqueue_restart(), which can restart the
> > > > virtqueue when the vhost has already started running.
> > > >
> > > > Meanwhile, vhost_dev_virtqueue_release(), which can ummap the
> > > > vrings and the desc of a specific vq of a device.
> > > >
> > > > Combining the two functions, we can reset a virtqueue with a
> > > > started vhost.
> > > >
> > > > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >    hw/virtio/vhost.c         | 29 +++++++++++++++++++++++++++++
> > > >    include/hw/virtio/vhost.h |  6 ++++++
> > > >    2 files changed, 35 insertions(+)
> > > >
> > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > index e467dfc7bc..d158d71866 100644
> > > > --- a/hw/virtio/vhost.c
> > > > +++ b/hw/virtio/vhost.c
> > > > @@ -1904,3 +1904,32 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> > > >          return -ENOSYS;
> > > >    }
> > > > +
> > > > +void vhost_dev_virtqueue_release(struct vhost_dev *hdev,
> > > > VirtIODevice *vdev,
> > > > +                                 int vq_index)
> > > > +{
> > > > +    int idx = vq_index - hdev->vq_index;
> > > > +
> > > > +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
> > > > +
> > > > +    vhost_virtqueue_unmap(hdev,
> > > > +                          vdev,
> > > > +                          hdev->vqs + idx,
> > > > +                          hdev->vq_index + idx);
> > > > +}
> > >
> > > Anything wrong that makes you can't use vhost_virtqueue_stop() here?
> > >
> > > Thanks
> > Yeah, vhost_virtqueue_stop() will result in all the queues in the device
> > being stopped.
> This sounds a defect of the backend. I think we'd better not
> workaround it in the qemu.
Yeah, I agree.
> > In the scenario of DPDK-OVS, vhost_virtqueue_stop will send
> > VHOST_USER_GET_VRING_BASE to DPDK. This message is meant to destroy the
> > queue.
> That's tricky.
>
> > However, In DPDK, VHOST_USER_GET_VRING_BASE message will lead to
> > vhost_destroy_device_notify(). This function will disable all the queues
> > in the device.
> Ok. I wonder how the reset is implemented in DPDK? It looks to me a
> new type of request is required.
Currently, DPDK does not support reset of queue level, but it supports 
device-level reset. VHOST_USER_RESET_DEVICE request exists in vhost 
protocol but it is not used in vhost-net module in QEMU.

It seems like there is no way for the driver to directly reset the 
device in vhost user net (If we use ethtool -r <eth_name>, we will get 
operation not supported.)

Actually we can plug the driver in and out to perform a device reset 
operation, but it will be applied to all the devices. I take it for 
example here.

If you plug out the driver(modprobe -r virtio-net), vhost_dev_stop() 
will be called in qemu. DPDK will receive VHOST_USER_GET_VRING_BASE 
messages for each virtqueue.

Then you can plug in the driver(modprobe virtio-net), vhost_dev_start() 
will be called in qemu. The process is same as when we boot the machine, 
DPDK will receive:

    VHOST_USER_SET_VRING_NUM, VHOST_USER_SET_VRING_BASE, 
VHOST_USER_SET_VRING_ADDR, VHOST_USER_SET_VRING_KICK, 
VHOST_USER_SET_VRING_CALL

in order to start a virtqueue.

Our vq reset implemenation does not need any change to the DPDK except 
adding the feature bit, since we reuse all the request types in 
vhost-user protocol.

I agree that we should add a new request in vhost-user protocol to 
support vq reset, we'll implement it in the next version.

> > Thus, when restarting the queue, other queues except the restarted one
> > are still disabled.
> >
> > On the other hand, I think it may be not necessary to destroy the queue.
> > We can simply disable it. When restarting the queue, it will be
> > intialized again.
> So for vhost-net kernel, we can simply set NULL as the backend for a
> specific virtqueue. We just need to refactor the stop codes (which did
> it at vhost device level instead of queue level currently).
>
> Thanks

Yes, I think so.

Thanks

> > Thanks
> >
> > > > +
> > > > +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice
> > > > *vdev,
> > > > +                                int vq_index)
> > > > +{
> > > > +    int idx = vq_index - hdev->vq_index;
> > > > +    int r = 0;
> > > > +
> > > > +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
> > > > +
> > > > +    r = vhost_virtqueue_start(hdev,
> > > > +                              vdev,
> > > > +                              hdev->vqs + idx,
> > > > +                              hdev->vq_index + idx);
> > > > +
> > > > +    return r;
> > > > +}
> > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > > index a346f23d13..7df7dbe24d 100644
> > > > --- a/include/hw/virtio/vhost.h
> > > > +++ b/include/hw/virtio/vhost.h
> > > > @@ -277,6 +277,12 @@ bool vhost_has_free_slot(void);
> > > >    int vhost_net_set_backend(struct vhost_dev *hdev,
> > > >                              struct vhost_vring_file *file);
> > > >    +
> > > > +void vhost_dev_virtqueue_release(struct vhost_dev *hdev,
> > > > VirtIODevice *vdev,
> > > > +                                 int vq_index);
> > > > +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice
> > > > *vdev,
> > > > +                                int vq_index);
> > > > +
> > > >    int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova,
> > > > int write);
> > > >      void vhost_dev_reset_inflight(struct vhost_inflight *inflight);