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

[Maxime Leroy]

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