[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/2] vhost-user: add multi queue support
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/2] vhost-user: add multi queue support |
Date: |
Thu, 13 Aug 2015 13:55:51 +0300 |
On Thu, Aug 13, 2015 at 12:24:16PM +0200, Maxime Leroy wrote:
> On Thu, Aug 13, 2015 at 11:18 AM, Michael S. Tsirkin <address@hidden> wrote:
> > On Wed, Aug 12, 2015 at 02:25:41PM +0800, Ouyang Changchun wrote:
> >> Based on patch by Nikolay Nikolaev:
> >> Vhost-user will implement the multi queue support in a similar way
> >> to what vhost already has - a separate thread for each queue.
> >> To enable the multi queue functionality - a new command line parameter
> >> "queues" is introduced for the vhost-user netdev.
> >>
> >> The RESET_OWNER change is based on commit:
> >> 294ce717e0f212ed0763307f3eab72b4a1bdf4d0
> >> If it is reverted, the patch need update for it accordingly.
> >>
> >> Signed-off-by: Nikolay Nikolaev <address@hidden>
> >> Signed-off-by: Changchun Ouyang <address@hidden>
> >> ---
> >> Changes since v5:
> >> - fix the message descption for VHOST_RESET_OWNER in vhost-user txt
> >>
> >> Changes since v4:
> >> - remove the unnecessary trailing '\n'
> >>
> >> Changes since v3:
> >> - fix one typo and wrap one long line
> >>
> >> Changes since v2:
> >> - fix vq index issue for set_vring_call
> >> When it is the case of VHOST_SET_VRING_CALL, The vq_index is not
> >> initialized before it is used,
> >> thus it could be a random value. The random value leads to crash in
> >> vhost after passing down
> >> to vhost, as vhost use this random value to index an array index.
> >> - fix the typo in the doc and description
> >> - address vq index for reset_owner
> >>
> >> Changes since v1:
> >> - use s->nc.info_str when bringing up/down the backend
> >>
> >> docs/specs/vhost-user.txt | 7 ++++++-
> >> hw/net/vhost_net.c | 3 ++-
> >> hw/virtio/vhost-user.c | 11 ++++++++++-
> >> net/vhost-user.c | 37 ++++++++++++++++++++++++-------------
> >> qapi-schema.json | 6 +++++-
> >> qemu-options.hx | 5 +++--
> >> 6 files changed, 50 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> >> index 70da3b1..9390f89 100644
> >> --- a/docs/specs/vhost-user.txt
> >> +++ b/docs/specs/vhost-user.txt
> >> @@ -135,6 +135,11 @@ As older slaves don't support negotiating protocol
> >> features,
> >> a feature bit was dedicated for this purpose:
> >> #define VHOST_USER_F_PROTOCOL_FEATURES 30
> >>
> >> +Multi queue support
> >> +-------------------
> >> +The protocol supports multiple queues by setting all index fields in the
> >> sent
> >> +messages to a properly calculated value.
> >> +
> >> Message types
> >> -------------
> >>
> >> @@ -198,7 +203,7 @@ Message types
> >>
> >> Id: 4
> >> Equivalent ioctl: VHOST_RESET_OWNER
> >> - Master payload: N/A
> >> + Master payload: vring state description
> >>
> >> Issued when a new connection is about to be closed. The Master will
> >> no
> >> longer own this connection (and will usually close it).
> >
> > This is an interface change, isn't it?
> > We can't make it unconditionally, need to make it dependent
> > on a protocol flag.
>
> Agree. It can potential break vhost-user driver implementation
> checking the size of the message. We should not change the vhost-user
> protocol without a new protocol flag.
>
> I think the first issue here that VHOST_RESET_OWNER should happen on
> vhost_dev_cleanup and not in vhost_net_stop_one.
>
> VHOST_RESET_OWNER should be the counter part of VHOST_SET_OWNER. So it
> don't need to have a payload like VHOST_SET_OWNER.
>
> Thus I agree with this email
> (http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg05971.html)
>
> Maybe should we use an other message to tell to the backend that the
> vring is not anymore available in vhost_net_stop_one ?
>
> Maxime
I think the cleanest fix is to rename this message to e.g.
VHOST_RESET_DEVICE. This way we won't break existing users.
--
MST
- [Qemu-devel] [PATCH v6 0/2] vhost-user multi queue support, Ouyang Changchun, 2015/08/12
- [Qemu-devel] [PATCH v6 1/2] vhost-user: add multi queue support, Ouyang Changchun, 2015/08/12
- Re: [Qemu-devel] [PATCH v6 1/2] vhost-user: add multi queue support, Michael S. Tsirkin, 2015/08/13
- Re: [Qemu-devel] [snabb-devel] Re: [PATCH v6 1/2] vhost-user: add multi queue support, Ouyang, Changchun, 2015/08/24
- Re: [Qemu-devel] [snabb-devel] Re: [PATCH v6 1/2] vhost-user: add multi queue support, Michael S. Tsirkin, 2015/08/27
- Re: [Qemu-devel] [snabb-devel] Re: [PATCH v6 1/2] vhost-user: add multi queue support, Ouyang, Changchun, 2015/08/27
- Re: [Qemu-devel] [snabb-devel] Re: [PATCH v6 1/2] vhost-user: add multi queue support, Michael S. Tsirkin, 2015/08/30
- Re: [Qemu-devel] [snabb-devel] Re: [PATCH v6 1/2] vhost-user: add multi queue support, Ouyang, Changchun, 2015/08/31
- Re: [Qemu-devel] [snabb-devel] Re: [PATCH v6 1/2] vhost-user: add multi queue support, Michael S. Tsirkin, 2015/08/31
- Re: [Qemu-devel] [snabb-devel] Re: [PATCH v6 1/2] vhost-user: add multi queue support, Eric Blake, 2015/08/31
[Qemu-devel] [PATCH v6 2/2] vhost-user: new protocol feature for multi queue, Ouyang Changchun, 2015/08/12