Re: [dpdk-dev] [PATCH v7 2/4] vhost: add support for packed ring in async vhost

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 4/14/21 8:13 AM, Cheng Jiang wrote:
> For now async vhost data path only supports split ring. This patch
> enables packed ring in async vhost data path to make async vhost
> compatible with virtio 1.1 spec.
> 
> Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> ---
>  lib/librte_vhost/rte_vhost_async.h |   1 +
>  lib/librte_vhost/vhost.c           |  49 ++--
>  lib/librte_vhost/vhost.h           |  15 +-
>  lib/librte_vhost/virtio_net.c      | 432 +++++++++++++++++++++++++++--
>  4 files changed, 456 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vhost_async.h b/lib/librte_vhost/rte_vhost_async.h
> index c855ff875..6faa31f5a 100644
> --- a/lib/librte_vhost/rte_vhost_async.h
> +++ b/lib/librte_vhost/rte_vhost_async.h
> @@ -89,6 +89,7 @@ struct rte_vhost_async_channel_ops {
>  struct async_inflight_info {
>  	struct rte_mbuf *mbuf;
>  	uint16_t descs; /* num of descs inflight */
> +	uint16_t nr_buffers; /* num of buffers inflight for packed ring */
>  };
>  
>  /**
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index a70fe01d8..f509186c6 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -338,19 +338,22 @@ cleanup_device(struct virtio_net *dev, int destroy)
>  }
>  
>  static void
> -vhost_free_async_mem(struct vhost_virtqueue *vq)
> +vhost_free_async_mem(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  {
> -	if (vq->async_pkts_info)
> -		rte_free(vq->async_pkts_info);
> -	if (vq->async_descs_split)
> +	rte_free(vq->async_pkts_info);
> +
> +	if (vq_is_packed(dev)) {
> +		rte_free(vq->async_buffers_packed);
> +		vq->async_buffers_packed = NULL;
> +	} else {

Doing this is not necessary:

	rte_free(vq->async_buffers_packed);
	vq->async_buffers_packed = NULL;
	rte_free(vq->async_descs_split);
	vq->async_descs_split = NULL;

Above will just work and will avoid adding dev parameter.


>  		rte_free(vq->async_descs_split);
> -	if (vq->it_pool)
> -		rte_free(vq->it_pool);
> -	if (vq->vec_pool)
> -		rte_free(vq->vec_pool);
> +		vq->async_descs_split = NULL;
> +	}
> +
> +	rte_free(vq->it_pool);
> +	rte_free(vq->vec_pool);
>  
>  	vq->async_pkts_info = NULL;
> -	vq->async_descs_split = NULL;
>  	vq->it_pool = NULL;
>  	vq->vec_pool = NULL;
>  }
> @@ -360,10 +363,10 @@ free_vq(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  {
>  	if (vq_is_packed(dev))
>  		rte_free(vq->shadow_used_packed);
> -	else {
> +	else
>  		rte_free(vq->shadow_used_split);
> -		vhost_free_async_mem(vq);
> -	}
> +
> +	vhost_free_async_mem(dev, vq);
>  	rte_free(vq->batch_copy_elems);
>  	if (vq->iotlb_pool)
>  		rte_mempool_free(vq->iotlb_pool);
> @@ -1626,10 +1629,9 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	if (unlikely(vq == NULL || !dev->async_copy))
>  		return -1;
>  
> -	/* packed queue is not supported */
> -	if (unlikely(vq_is_packed(dev) || !f.async_inorder)) {
> +	if (unlikely(!f.async_inorder)) {
>  		VHOST_LOG_CONFIG(ERR,
> -			"async copy is not supported on packed queue or non-inorder mode "
> +			"async copy is not supported on non-inorder mode "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
>  		return -1;
>  	}
> @@ -1667,12 +1669,19 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	vq->vec_pool = rte_malloc_socket(NULL,
>  			VHOST_MAX_ASYNC_VEC * sizeof(struct iovec),
>  			RTE_CACHE_LINE_SIZE, node);
> -	vq->async_descs_split = rte_malloc_socket(NULL,
> +	if (vq_is_packed(dev)) {
> +		vq->async_buffers_packed = rte_malloc_socket(NULL,
> +			vq->size * sizeof(struct vring_used_elem_packed),
> +			RTE_CACHE_LINE_SIZE, node);
> +	} else {
> +		vq->async_descs_split = rte_malloc_socket(NULL,
>  			vq->size * sizeof(struct vring_used_elem),
>  			RTE_CACHE_LINE_SIZE, node);
> -	if (!vq->async_descs_split || !vq->async_pkts_info ||
> -		!vq->it_pool || !vq->vec_pool) {
> -		vhost_free_async_mem(vq);
> +	}
> +
> +	if (!vq->async_buffers_packed || !vq->async_descs_split ||
> +		!vq->async_pkts_info || !vq->it_pool || !vq->vec_pool) {

Not really than of this error handling. Checking after every malloc if
it suceed would be cleaner.

> +		vhost_free_async_mem(dev, vq);
>  		VHOST_LOG_CONFIG(ERR,
>  				"async register failed: cannot allocate memory for vq data "
>  				"(vid %d, qid: %d)\n", vid, queue_id);
> @@ -1728,7 +1737,7 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
>  		goto out;
>  	}
>  
> -	vhost_free_async_mem(vq);
> +	vhost_free_async_mem(dev, vq);
>  
>  	vq->async_ops.transfer_data = NULL;
>  	vq->async_ops.check_completed_copies = NULL;
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index f628714c2..673335217 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -201,9 +201,18 @@ struct vhost_virtqueue {
>  	uint16_t	async_pkts_idx;
>  	uint16_t	async_pkts_inflight_n;
>  	uint16_t	async_last_pkts_n;
> -	struct vring_used_elem  *async_descs_split;
> -	uint16_t async_desc_idx;
> -	uint16_t last_async_desc_idx;
> +	union {
> +		struct vring_used_elem  *async_descs_split;
> +		struct vring_used_elem_packed *async_buffers_packed;
> +	};
> +	union {
> +		uint16_t async_desc_idx;
> +		uint16_t async_packed_buffer_idx;
> +	};
> +	union {
> +		uint16_t last_async_desc_idx;
> +		uint16_t last_async_buffer_idx;
> +	};

Looks almost good to me now, thanks for doing the change.
Only minor issue is the naming which is not consistent in the different
fields. Sometimes it contains split or packed, sometimes not.

Maxime