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

[Michael S. Tsirkin]

On Wed, Oct 21, 2015 at 10:55:39PM +0800, Yuanhan Liu wrote:
> On Wed, Oct 21, 2015 at 05:11:44PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 21, 2015 at 09:43:16PM +0800, Yuanhan Liu wrote:
> > > 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 :(
> > 
> > It's not well specified, but it does say it's analogous to RESET_OWNER
> > in kernel. That one is well documented:
> > 
> > /* Set current process as the (exclusive) owner of this file descriptor.
> >  * This must be called before any other vhost command.  Further calls to
> >  * VHOST_OWNER_SET fail until VHOST_OWNER_RESET is called. */
> > #define VHOST_SET_OWNER _IO(VHOST_VIRTIO, 0x01)
> > /* Give up ownership, and reset the device to default values.
> >  * Allows subsequent call to VHOST_OWNER_SET to succeed. */
> > #define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
> 
> Thanks, that helps (I think).
> 
> I recalled my old question, and rechecked your answer again:
> 
>     Because we need to get the state from remote after stop.
>     RESET_OWNER discards that, so we can't resume the VM.
> 
> So, if I understand it correctly this time, you want to keep the
> VM state at the backend side even after the VM is shut down, and
> then we can resume it with those saved state

Not shut down. That makes no sense. When VM is stopped.

> And why don't do that when MQ is enabled? I don't see it has anyting
> to do with MQ.

With MQ we have enable/disable vq so we can just stop them
cleanly.

> > 
> > So if we want just the reset part, we need to do VHOST_RESET_OWNER
> > then redo everything that we did previously: VHOST_SET_OWNER
> > SET_VRING_CALL etc etc.
> > 
> > > 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 ...):
> > 
> > Food for thought.
> 
> Aha...
> 
> > 
> > > 
> > >     # 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.
> > > 
> > 
> > reboot, not shutdown.
> 
> I see.
> 
> 
> According to the log, virtio_net_reset() doesn't seem to the right
> place, as you can see, SET_OWNER is invoked before RESET_OWNER.
> 
> > 
> > 
> > > > 
> > > > > 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 <address@hidden>
> > > > > Signed-off-by: Yuanhan Liu <address@hidden>
> > > > > ---
> > > > >  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
> > 
> > I prefer an if condition. Seems easier to follow.
> 
> Ok.
> 
>       --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
> > > > >