Re: [dpdk-dev] [PATCH v4 02/12] vhost: support multiple queues in virtio dev

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

Hi Changchun,

On Wed, Aug 12, 2015 at 04:02:37PM +0800, Ouyang Changchun wrote:
> Each virtio device could have multiple queues, say 2 or 4, at most 8.
> Enabling this feature allows virtio device/port on guest has the ability to
> use different vCPU to receive/transmit packets from/to each queue.
> 
> In multiple queues mode, virtio device readiness means all queues of
> this virtio device are ready, cleanup/destroy a virtio device also
> requires clearing all queues belong to it.
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
[snip ..]
>  /*
> + *  Initialise all variables in vring queue pair.
> + */
> +static void
> +init_vring_queue_pair(struct virtio_net *dev, uint16_t qp_idx)
> +{
> +	uint32_t virt_rx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_RXQ;
> +	uint32_t virt_tx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_TXQ;
> +	memset(dev->virtqueue[virt_rx_q_idx], 0, sizeof(struct vhost_virtqueue));
> +	memset(dev->virtqueue[virt_tx_q_idx], 0, sizeof(struct vhost_virtqueue));
> +
> +	dev->virtqueue[virt_rx_q_idx]->kickfd = (eventfd_t)-1;
> +	dev->virtqueue[virt_rx_q_idx]->callfd = (eventfd_t)-1;
> +	dev->virtqueue[virt_tx_q_idx]->kickfd = (eventfd_t)-1;
> +	dev->virtqueue[virt_tx_q_idx]->callfd = (eventfd_t)-1;
> +
> +	/* Backends are set to -1 indicating an inactive device. */
> +	dev->virtqueue[virt_rx_q_idx]->backend = VIRTIO_DEV_STOPPED;
> +	dev->virtqueue[virt_tx_q_idx]->backend = VIRTIO_DEV_STOPPED;
> +}
> +
> +/*
>   *  Initialise all variables in device structure.
>   */
>  static void
> @@ -258,17 +294,34 @@ init_device(struct virtio_net *dev)
>  	/* Set everything to 0. */

There is a trick here. Let me fill the context first:

283 static void
284 init_device(struct virtio_net *dev)
285 {
286         uint64_t vq_offset;
287
288         /*
289          * Virtqueues have already been malloced so
290          * we don't want to set them to NULL.
291          */
292         vq_offset = offsetof(struct virtio_net, mem);
293
294         /* Set everything to 0. */
295         memset((void *)(uintptr_t)((uint64_t)(uintptr_t)dev + vq_offset), 0,
296                 (sizeof(struct virtio_net) - (size_t)vq_offset));
297
298         init_vring_queue_pair(dev, 0);

This piece of code's intention is to memset everything to zero, except
the `virtqueue' field, for, as the comment stated, we have already
allocated virtqueue.

It works only when `virtqueue' field is before `mem' field, and it was
before:

    struct virtio_net {
            struct vhost_virtqueue  *virtqueue[VIRTIO_QNUM];        /**< Contains all virtqueue information. */
            struct virtio_memory    *mem;           /**< QEMU memory and memory region information. */
            ...

After this patch, it becomes:

    struct virtio_net {
            struct virtio_memory    *mem;           /**< QEMU memory and memory region information. */
            struct vhost_virtqueue  **virtqueue;    /**< Contains all virtqueue information. */
            ...


Which actually wipes all stuff inside `struct virtio_net`, resulting to 
setting `virtqueue' to NULL as well.

While reading the code(without you patch applied), I thought that it's
error-prone, as it is very likely that someone else besides the author
doesn't know such undocumented rule. And you just gave me an example :)

Huawei, I'm proposing a fix to call rte_zmalloc() for allocating new_ll_dev
to get rid of such issue. What do you think?

	--yliu



>  	memset((void *)(uintptr_t)((uint64_t)(uintptr_t)dev + vq_offset), 0,
>  		(sizeof(struct virtio_net) - (size_t)vq_offset));
> -	memset(dev->virtqueue[VIRTIO_RXQ], 0, sizeof(struct vhost_virtqueue));
> -	memset(dev->virtqueue[VIRTIO_TXQ], 0, sizeof(struct vhost_virtqueue));
>  
> -	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
> -	dev->virtqueue[VIRTIO_RXQ]->callfd = (eventfd_t)-1;
> -	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
> -	dev->virtqueue[VIRTIO_TXQ]->callfd = (eventfd_t)-1;
> +	init_vring_queue_pair(dev, 0);
> +	dev->virt_qp_nb = 1;
> +}
> +
> +/*
> + *  Alloc mem for vring queue pair.
> + */
> +int
> +alloc_vring_queue_pair(struct virtio_net *dev, uint16_t qp_idx)
> +{
> +	struct vhost_virtqueue *virtqueue = NULL;
> +	uint32_t virt_rx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_RXQ;
> +	uint32_t virt_tx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_TXQ;
>  
> -	/* Backends are set to -1 indicating an inactive device. */
> -	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
> -	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
> +	virtqueue = rte_malloc(NULL, sizeof(struct vhost_virtqueue) * VIRTIO_QNUM, 0);
> +	if (virtqueue == NULL) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"Failed to allocate memory for virt qp:%d.\n", qp_idx);
> +		return -1;
> +	}
> +
> +	dev->virtqueue[virt_rx_q_idx] = virtqueue;
> +	dev->virtqueue[virt_tx_q_idx] = virtqueue + VIRTIO_TXQ;
> +
> +	init_vring_queue_pair(dev, qp_idx);
> +
> +	return 0;
>  }
>  
>  /*
> @@ -280,7 +333,6 @@ static int
>  new_device(struct vhost_device_ctx ctx)
>  {
>  	struct virtio_net_config_ll *new_ll_dev;
> -	struct vhost_virtqueue *virtqueue_rx, *virtqueue_tx;
>  
>  	/* Setup device and virtqueues. */
>  	new_ll_dev = rte_malloc(NULL, sizeof(struct virtio_net_config_ll), 0);
> @@ -291,28 +343,22 @@ new_device(struct vhost_device_ctx ctx)
>  		return -1;
>  	}
>  
> -	virtqueue_rx = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> -	if (virtqueue_rx == NULL) {
> -		rte_free(new_ll_dev);
> +	new_ll_dev->dev.virtqueue =
> +		rte_malloc(NULL, VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX * sizeof(struct vhost_virtqueue *), 0);
> +	if (new_ll_dev->dev.virtqueue == NULL) {
>  		RTE_LOG(ERR, VHOST_CONFIG,
> -			"(%"PRIu64") Failed to allocate memory for rxq.\n",
> +			"(%"PRIu64") Failed to allocate memory for dev.virtqueue.\n",
>  			ctx.fh);
> +		rte_free(new_ll_dev);
>  		return -1;
>  	}
>  
> -	virtqueue_tx = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> -	if (virtqueue_tx == NULL) {
> -		rte_free(virtqueue_rx);
> +	if (alloc_vring_queue_pair(&new_ll_dev->dev, 0) == -1) {
> +		rte_free(new_ll_dev->dev.virtqueue);
>  		rte_free(new_ll_dev);
> -		RTE_LOG(ERR, VHOST_CONFIG,
> -			"(%"PRIu64") Failed to allocate memory for txq.\n",
> -			ctx.fh);
>  		return -1;
>  	}
>  
> -	new_ll_dev->dev.virtqueue[VIRTIO_RXQ] = virtqueue_rx;
> -	new_ll_dev->dev.virtqueue[VIRTIO_TXQ] = virtqueue_tx;
> -
>  	/* Initialise device and virtqueues. */
>  	init_device(&new_ll_dev->dev);
>  
> @@ -396,7 +442,7 @@ set_owner(struct vhost_device_ctx ctx)
>   * Called from CUSE IOCTL: VHOST_RESET_OWNER
>   */
>  static int
> -reset_owner(struct vhost_device_ctx ctx)
> +reset_owner(__rte_unused struct vhost_device_ctx ctx)
>  {
>  	struct virtio_net_config_ll *ll_dev;
>  
> @@ -434,6 +480,7 @@ static int
>  set_features(struct vhost_device_ctx ctx, uint64_t *pu)
>  {
>  	struct virtio_net *dev;
> +	uint32_t q_idx;
>  
>  	dev = get_device(ctx);
>  	if (dev == NULL)
> @@ -445,22 +492,26 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
>  	dev->features = *pu;
>  
>  	/* Set the vhost_hlen depending on if VIRTIO_NET_F_MRG_RXBUF is set. */
> -	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> -		LOG_DEBUG(VHOST_CONFIG,
> -			"(%"PRIu64") Mergeable RX buffers enabled\n",
> -			dev->device_fh);
> -		dev->virtqueue[VIRTIO_RXQ]->vhost_hlen =
> -			sizeof(struct virtio_net_hdr_mrg_rxbuf);
> -		dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =
> -			sizeof(struct virtio_net_hdr_mrg_rxbuf);
> -	} else {
> -		LOG_DEBUG(VHOST_CONFIG,
> -			"(%"PRIu64") Mergeable RX buffers disabled\n",
> -			dev->device_fh);
> -		dev->virtqueue[VIRTIO_RXQ]->vhost_hlen =
> -			sizeof(struct virtio_net_hdr);
> -		dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =
> -			sizeof(struct virtio_net_hdr);
> +	for (q_idx = 0; q_idx < dev->virt_qp_nb; q_idx++) {
> +		uint32_t virt_rx_q_idx = q_idx * VIRTIO_QNUM + VIRTIO_RXQ;
> +		uint32_t virt_tx_q_idx = q_idx * VIRTIO_QNUM + VIRTIO_TXQ;
> +		if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> +			LOG_DEBUG(VHOST_CONFIG,
> +				"(%"PRIu64") Mergeable RX buffers enabled\n",
> +				dev->device_fh);
> +			dev->virtqueue[virt_rx_q_idx]->vhost_hlen =
> +				sizeof(struct virtio_net_hdr_mrg_rxbuf);
> +			dev->virtqueue[virt_tx_q_idx]->vhost_hlen =
> +				sizeof(struct virtio_net_hdr_mrg_rxbuf);
> +		} else {
> +			LOG_DEBUG(VHOST_CONFIG,
> +				"(%"PRIu64") Mergeable RX buffers disabled\n",
> +				dev->device_fh);
> +			dev->virtqueue[virt_rx_q_idx]->vhost_hlen =
> +				sizeof(struct virtio_net_hdr);
> +			dev->virtqueue[virt_tx_q_idx]->vhost_hlen =
> +				sizeof(struct virtio_net_hdr);
> +		}
>  	}
>  	return 0;
>  }
> @@ -826,6 +877,14 @@ int rte_vhost_feature_enable(uint64_t feature_mask)
>  	return -1;
>  }
>  
> +uint16_t rte_vhost_qp_num_get(struct virtio_net *dev)
> +{
> +	if (dev == NULL)
> +		return 0;
> +
> +	return dev->virt_qp_nb;
> +}
> +
>  /*
>   * Register ops so that we can add/remove device to data core.
>   */
> -- 
> 1.8.4.2
>