Re: [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/10/22 21:35, Yuanhan Liu wrote:
> All queue pairs, including the default (the first) queue pair,
> are allocated dynamically, when a vring_call message is received
> first time for a specific queue pair.
>
> This is a refactor work for enabling vhost-user multiple queue;
> it should not break anything as it does no functional changes:
> we don't support mq set, so there is only one mq at max.
>
> This patch is based on Changchun's patch.
>
> Signed-off-by: Ouyang Changchun <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Acked-by: Flavio Leitner <fbl@sysclose.org>
>
> ---
>
> v8: - move virtuque field to the end of `virtio_net' struct.
>
>     - Add a FIXME at set_vring_call() for doing vring queue pair
>       allocation.
> ---
>  lib/librte_vhost/rte_virtio_net.h             |   3 +-
>  lib/librte_vhost/vhost_user/virtio-net-user.c |  46 ++++----
>  lib/librte_vhost/virtio-net.c                 | 156 ++++++++++++++++----------
>  3 files changed, 123 insertions(+), 82 deletions(-)
>
> diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
> index e3a21e5..9a32a95 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -96,7 +96,6 @@ struct vhost_virtqueue {
>   * Device structure contains all configuration information relating to the device.
>   */
>  struct virtio_net {
> -	struct vhost_virtqueue	*virtqueue[VIRTIO_QNUM];	/**< Contains all virtqueue information. */
>  	struct virtio_memory	*mem;		/**< QEMU memory and memory region information. */
>  	uint64_t		features;	/**< Negotiated feature set. */
>  	uint64_t		protocol_features;	/**< Negotiated protocol feature set. */
> @@ -104,7 +103,9 @@ struct virtio_net {
>  	uint32_t		flags;		/**< Device flags. Only used to check if device is running on data core. */
>  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>  	char			ifname[IF_NAME_SZ];	/**< Name of the tap device or socket path. */
> +	uint32_t		virt_qp_nb;	/**< number of queue pair we have allocated */
>  	void			*priv;		/**< private context */
> +	struct vhost_virtqueue	*virtqueue[VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX];	/**< Contains all virtqueue information. */
>  } __rte_cache_aligned;
>  
>  /**
> diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c b/lib/librte_vhost/vhost_user/virtio-net-user.c
> index 6da729d..d62f3d7 100644
> --- a/lib/librte_vhost/vhost_user/virtio-net-user.c
> +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
> @@ -206,25 +206,33 @@ err_mmap:
>  }
>  
>  static int
> +vq_is_ready(struct vhost_virtqueue *vq)
> +{
> +	return vq && vq->desc   &&
> +	       vq->kickfd != -1 &&
> +	       vq->callfd != -1;
> +}
> +
> +static int
>  virtio_is_ready(struct virtio_net *dev)
>  {
>  	struct vhost_virtqueue *rvq, *tvq;
> +	uint32_t i;
>  
> -	/* mq support in future.*/
> -	rvq = dev->virtqueue[VIRTIO_RXQ];
> -	tvq = dev->virtqueue[VIRTIO_TXQ];
> -	if (rvq && tvq && rvq->desc && tvq->desc &&
> -		(rvq->kickfd != -1) &&
> -		(rvq->callfd != -1) &&
> -		(tvq->kickfd != -1) &&
> -		(tvq->callfd != -1)) {
> -		RTE_LOG(INFO, VHOST_CONFIG,
> -			"virtio is now ready for processing.\n");
> -		return 1;
> +	for (i = 0; i < dev->virt_qp_nb; i++) {
> +		rvq = dev->virtqueue[i * VIRTIO_QNUM + VIRTIO_RXQ];
> +		tvq = dev->virtqueue[i * VIRTIO_QNUM + VIRTIO_TXQ];
> +
> +		if (!vq_is_ready(rvq) || !vq_is_ready(tvq)) {
> +			RTE_LOG(INFO, VHOST_CONFIG,
> +				"virtio is not ready for processing.\n");
> +			return 0;
> +		}
>  	}
> +
>  	RTE_LOG(INFO, VHOST_CONFIG,
> -		"virtio isn't ready for processing.\n");
> -	return 0;
> +		"virtio is now ready for processing.\n");
> +	return 1;
>  }
>  
>  void
> @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
>  	 * sent and only sent in vhost_vring_stop.
>  	 * TODO: cleanup the vring, it isn't usable since here.
>  	 */
> -	if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> -		close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> -		dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> +	if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> +		close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
> +		dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
>  	}

Hi Yuanhan,

Please let me make sure whether below is correct.
    if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {

> -	if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> -		close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> -		dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
> +	if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
> +		close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);
> +		dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;

Also, same question here.

Thanks,
Tetsuya