Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Wed, Oct 21, 2015 at 01:39:11PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 05:07:18PM +0800, Yuanhan Liu wrote:
> > Send VHOST_USER_SET_VRING_ENABLE at start/stop when multiple queue
> > is negotiated, to inform the backend that we are ready or not.
> 
> OK but that's only if MQ is set.

Maybe we could just call vhost_backend_set_vring_enable() unconditionally?
It's nil operation when MQ is not set.

> If now, we need to do
> RESET_OWNER followed by SET_OWNER.

Could you be more specific? Why sending RESET_OWNER followed by
SET_OWNER?

TBH, I'm a bit confused with RESET_OWNER now: what it does, and when is
supposed to send it :(

And, sending RESET_OWNER inside virtio_net_reset() also looks weird.
I made a quick try before sending this patchset, and the vhost-user
request dump doesn't look right to me: the message is sent after
vhost dev init (GET_FEATURES, GET_PROTOCOL_FEATURE, SET_OWNER, ...,
SET_VRING_CALL),  and before peer attach (SET_VRING_ENABLE) and
vhost_dev_start (SET_MEM_TABLE, ... SET_VRING_KICK ...):


    # start of a VM

    VHOST_CONFIG: new virtio connection is 28
    VHOST_CONFIG: new device, handle is 0
    VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
    VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
    VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
    VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
    VHOST_CONFIG: read message VHOST_USER_SET_OWNER
    VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:0 file:29
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:1 file:30
    VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
    VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
    VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
    VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
    VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    ...
    ...
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:6 file:35
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:7 file:36

==> VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
    VHOST_CONFIG: read message VHOST_USER_RESET_OWNER

    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
    VHOST_CONFIG: set queue enable: 1 to qp idx: 0
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
    VHOST_CONFIG: set queue enable: 0 to qp idx: 2
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
    VHOST_CONFIG: set queue enable: 0 to qp idx: 4
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
    VHOST_CONFIG: set queue enable: 0 to qp idx: 6
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:0 file:29
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:1 file:30
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:2 file:31
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:3 file:32
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:4 file:33
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:5 file:34
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:6 file:35
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
    VHOST_CONFIG: vring call idx:7 file:36
    VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
    VHOST_CONFIG: read message VHOST_USER_SET_MEM_TABLE
    VHOST_CONFIG: mapped region 0 fd:37 to 0x2aaac0000000 sz:0xa0000 off:0x0
    VHOST_CONFIG: mapped region 1 fd:38 to 0x2aab00000000 sz:0x80000000 off:0xc0000
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
    VHOST_CONFIG: vring kick idx:0 file:39
    VHOST_CONFIG: virtio is not ready for processing.
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
    VHOST_CONFIG: vring kick idx:1 file:40
    VHOST_CONFIG: virtio is not ready for processing.
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
    VHOST_CONFIG: set queue enable: 1 to qp idx: 0
    VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
    VHOST_CONFIG: vring kick idx:2 file:41
    VHOST_CONFIG: virtio is not ready for processing.
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
    VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
    ...


And I didn't see RESET_OWNER is sent while I shutdown a VM.


> 
> > And exclude VHOST_USER_GET_QUEUE_NUM as one time request, as we need
> > to get max_queues for each vhost_dev.
> 
> Pls split this part out.
> 
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> >  hw/virtio/vhost-user.c |  1 -
> >  hw/virtio/vhost.c      | 18 ++++++++++++++++++
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 12a9104..6532a73 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -194,7 +194,6 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> >      case VHOST_USER_SET_OWNER:
> >      case VHOST_USER_RESET_OWNER:
> >      case VHOST_USER_SET_MEM_TABLE:
> > -    case VHOST_USER_GET_QUEUE_NUM:
> >          return true;
> >      default:
> >          return false;
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index c0ed5b2..54a4633 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1146,6 +1146,19 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >          }
> >      }
> >  
> > +    /*
> > +     * Send VHOST_USER_SET_VRING_ENABLE message when multiple queue
> > +     * is negotiated to inform the back end that we are ready.
> > +     *
> > +     * Only set enable to 1 for first queue pair, as we enable one
> > +     * queue pair by default.
> > +     */
> > +    if (hdev->max_queues > 1 &&
> 
> this makes no sense in the generic code.
> This is a work around for a protocol bug - belongs in
> vhost user internally.

Maybe we could add a dummy vhost_backend_set_vring_enable() for
vhost-kernel?

> And that's not the way to test this. MQ could be negotiated
> even if there is a single pair of queues.

Yeah, right. Just as stated, how about calling it unconditionally here?

	--yliu
> 
> > +        hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > +        hdev->vhost_ops->vhost_backend_set_vring_enable(hdev,
> > +                                                        hdev->vq_index == 0);
> > +    }
> > +
> >      return 0;
> >  fail_log:
> >      vhost_log_put(hdev, false);
> > @@ -1180,5 +1193,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> >      hdev->started = false;
> >      hdev->log = NULL;
> >      hdev->log_size = 0;
> > +
> > +    if (hdev->max_queues > 1 &&
> > +        hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > +        hdev->vhost_ops->vhost_backend_set_vring_enable(hdev, 0);
> > +    }
> >  }
> >  
> > -- 
> > 1.9.0
> >