Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Jason Wang]

On Wed, Jul 27, 2022 at 3:05 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/27 12:55, Jason Wang 写道:
> > On Tue, Jul 26, 2022 at 2:39 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> 
> > wrote:
> >>
> >> 在 2022/7/26 11:49, Jason Wang 写道:
> >>> 在 2022/7/18 19:17, Kangjie Xu 写道:
> >>>> The interface to set enable status for a single vring is lacked in
> >>>> VhostOps, since the vhost_set_vring_enable_op will manipulate all
> >>>> virtqueues in a device.
> >>>>
> >>>> Resetting a single vq will rely on this interface. It requires a
> >>>> reply to indicate that the reset operation is finished, so the
> >>>> parameter, wait_for_reply, is added.
> >>>
> >>> The wait reply seems to be a implementation specific thing. Can we
> >>> hide it?
> >>>
> >>> Thanks
> >>>
> >> I do not hide wait_for_reply here because when stopping the queue, a
> >> reply is needed to ensure that the message has been processed and queue
> >> has been disabled.
> > This needs to be done at vhost-backend level instead of the general vhost 
> > code.
> >
> > E.g vhost-kernel or vDPA is using ioctl() which is synchronous.
> Yeah, I understand your concerns, will fix it in the next version.
> >> When restarting the queue, we do not need a reply, which is the same as
> >> what qemu does in vhost_dev_start().
> >>
> >> So I add this parameter to distinguish the two cases.
> >>
> >> I think one alternative implementation is to add a interface in
> >> VhostOps: queue_reset(). In this way details can be hidden. What do you
> >> think of this solution? Does it look better?
> > Let me ask it differently, under which case can we call this function
> > with wait_for_reply = false?
> >
> > Thanks
>
> Cases when we do not need to wait until that the reply has been
> processed. Actually in dev_start(), or dev_stop(),

But we don't need to send virtqueue reset requests in those cases?

> they do not wait for
> replies when enabling/disabling vqs.
>
> Specifically, vhost_set_vring_enable() will call it with wait_for_reply
> = false.
>
> In our vq reset scenario, it should be synchronous because the driver
> need to modify queues after the device stop using the vq in the backend.
> Undefined errors will occur If the device is still using the queue when
> the driver is making modifications to it,
>
> Back to our implementation, we will not expose this parameter in the
> next version.

Ok.

Thanks

>
> Thanks.
>
> >> Thanks
> >>
> >>>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>>> ---
> >>>>    include/hw/virtio/vhost-backend.h | 4 ++++
> >>>>    1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/include/hw/virtio/vhost-backend.h
> >>>> b/include/hw/virtio/vhost-backend.h
> >>>> index eab46d7f0b..7bddd1e9a0 100644
> >>>> --- a/include/hw/virtio/vhost-backend.h
> >>>> +++ b/include/hw/virtio/vhost-backend.h
> >>>> @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct
> >>>> vhost_dev *dev);
> >>>>    typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
> >>>>    typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
> >>>>    typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
> >>>> +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
> >>>> +                                                int index, int enable,
> >>>> +                                                bool wait_for_reply);
> >>>>    typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
> >>>>                                             int enable);
> >>>>    typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
> >>>> @@ -155,6 +158,7 @@ typedef struct VhostOps {
> >>>>        vhost_set_owner_op vhost_set_owner;
> >>>>        vhost_reset_device_op vhost_reset_device;
> >>>>        vhost_get_vq_index_op vhost_get_vq_index;
> >>>> +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
> >>>>        vhost_set_vring_enable_op vhost_set_vring_enable;
> >>>>        vhost_requires_shm_log_op vhost_requires_shm_log;
> >>>>        vhost_migration_done_op vhost_migration_done;
>