qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Yuanhan Liu
Subject: Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
Date: Wed, 21 Oct 2015 22:55:39 +0800
User-agent: Mutt/1.5.21 (2010-09-15)

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

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

> 
> 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
> > > > 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]