From: "Michael S. Tsirkin" <mst@redhat.com>
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
Date: Wed, 21 Oct 2015 17:11:44 +0300 [thread overview]
Message-ID: <20151021170441-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <20151021134316.GK3115@yliu-dev.sh.intel.com>
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)
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.
>
> # 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.
> >
> > > 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
I prefer an if condition. Seems easier to follow.
> >
> > > + 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
> > >
next prev parent reply other threads:[~2015-10-21 14:11 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-21 9:07 [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 2/5] doc: vhost-user: request naming fix Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 3/5] vhost-user-test: add multiple queue test Yuanhan Liu
2015-10-21 10:34 ` Michael S. Tsirkin
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop" Yuanhan Liu
2015-10-21 10:35 ` Michael S. Tsirkin
2015-10-21 10:39 ` Michael S. Tsirkin
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop Yuanhan Liu
2015-10-21 10:39 ` Michael S. Tsirkin
2015-10-21 13:43 ` Yuanhan Liu
2015-10-21 14:11 ` Michael S. Tsirkin [this message]
2015-10-21 14:55 ` Yuanhan Liu
2015-10-21 14:59 ` Michael S. Tsirkin
2015-10-21 10:40 ` [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Michael S. Tsirkin
2015-10-21 13:04 ` Yuanhan Liu
2015-10-21 14:13 ` Michael S. Tsirkin
2015-10-21 14:18 ` Yuanhan Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151021170441-mutt-send-email-mst@redhat.com \
--to=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=yuanhan.liu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).