Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5598 bytes --]

On 09/15/2015 01:10 AM, Yuanhan Liu wrote:
> From: Changchun Ouyang <changchun.ouyang@intel.com>
> 
> This patch is initially based a patch from Nikolay Nikolaev.
> 
> Here is the latest version for adding vhost-user multiple queue support,
> by creating a nc and vhost_net pair for each queue.

The phrase "Here is the latest version" doesn't make much sense in the
long term in git (that is, a year from now, we won't care how many
preliminary versions there were, just about the version that got
committed; and if anything in git changes after that point, it is no
longer the latest version).

> 
> Qemu exits if find that the backend can't support number of requested
> queues(by providing queues=# option). The max number is queried by a

space before ( in English.

> new message, VHOST_USER_GET_QUEUE_NUM, and is sent only when protocol
> feature VHOST_USER_PROTOCOL_F_MQ is present first.
> 
> The max queue check is done at vhost-user initiation stage. We initiate
> one queue first, which, in the meantime, also gets the max_queues the
> backend supports.
> 
> In older version, it was reported that some messages are sent more times
> than necessary. Here we came an agreement with Michael that we could
> categorize vhost user messages to 2 types: none-vring specific messages,

s/none/non/

> which should be sent only once, and vring specific messages, which should
> be sent per queue.
> 
> Here I introduced a helper function vhost_user_one_time_request(), which
> lists following messages as none-vring specific messages:

s/none/non/

> 
>         VHOST_USER_GET_FEATURES
>         VHOST_USER_SET_FEATURES
>         VHOST_USER_GET_PROTOCOL_FEATURES
>         VHOST_USER_SET_PROTOCOL_FEATURES
>         VHOST_USER_SET_OWNER
>         VHOST_USER_RESET_DEVICE
>         VHOST_USER_SET_MEM_TABLE
>         VHOST_USER_GET_QUEUE_NUM
> 
> For above messages, we simply ignore them when they are not sent the first
> time.
> 

Up to here is mostly fine for the commit message.  Meanwhile...

> v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
>     once only, and invoke qemu_find_net_clients_except() at the handler
>     to gather all related ncs, for initiating all queues. Which addresses
>     a hidden bug in older verion in a more QEMU-like way.
> 
> v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
>     value from net->nc->queue_index.

...this chunk here is useful only on the mailing list, and not in git,
and therefore, should appear...

> 
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---

...here, after the --- separator.

>  docs/specs/vhost-user.txt |  13 +++++
>  hw/net/vhost_net.c        |  10 ++--
>  hw/virtio/vhost-user.c    |  26 +++++++++
>  hw/virtio/vhost.c         |   5 +-
>  net/vhost-user.c          | 146 +++++++++++++++++++++++++++++++++-------------
>  qapi-schema.json          |   6 +-
>  qemu-options.hx           |   5 +-
>  7 files changed, 161 insertions(+), 50 deletions(-)
> 
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 43db9b4..acf5708 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
>  a feature bit was dedicated for this purpose:
>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
>  
> +Multiple queue support
> +-------------------

Nit: the ---- line looks better when it is as long as the text it is
highlighting (3 bytes more)

> +Multiple queue is treated as a protocol extension, hence the slave has to
> +implement protocol features first. Multiple queues is supported only when

Sounds awkward. Either "Multiple queues are supported" or "The multiple
queues feature is supported"

> +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.

space before ( in English.

> +
> +The max number of queues the slave supports can be queried with message
> +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of
> +requested queues is bigger than that.
> +
> +As all queues share one connection, the master uses a unique index for each
> +queue in the sent message to identify a specified queue.
> +
>  Message types
>  -------------
>  

> @@ -207,6 +224,15 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>          msg_request = request;
>      }
>  
> +    /*
> +     * For none-vring specific requests, like VHOST_USER_GET_FEATURES,

s/none/non/

> +     * we just need send it once in the first time. For later such
> +     * request, we just ignore it.
> +     */
> +    if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) {
> +        return 0;
> +    }

> +++ b/qapi-schema.json
> @@ -2480,12 +2480,16 @@
>  #
>  # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
>  #
> +# @queues: #optional number of queues to be created for multiqueue vhost-user
> +#          (default: 1) (Since 2.5)
> +#
>  # Since 2.1
>  ##
>  { 'struct': 'NetdevVhostUserOptions',
>    'data': {
>      'chardev':        'str',
> -    '*vhostforce':    'bool' } }
> +    '*vhostforce':    'bool',
> +    '*queues':        'int' } }

Interface looks okay.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]