Re: [Qemu-devel] [PATCH v6 1/2] vhost-user: add multi queue support

[Michael S. Tsirkin]

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