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

["Jiang, Cheng1" <cheng1.jiang@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, April 14, 2021 9:41 PM
> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Wang, Yinan <yinan.wang@intel.com>; Liu,
> Yong <yong.liu@intel.com>
> Subject: Re: [PATCH v7 2/4] vhost: add support for packed ring in async vhost
> 
> 
> 
> 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.

async_buffers_packed and async_descs_split are two members of a union.
If I rte_free(vq->async_buffers_packed), then there is no need to rte_free(vq->async_descs_split).
So I use dev to determine which one I need to free.
Sure, I can free then both with no condition(I can also only free any one of them), but I think it's a little strange, Right?

> 
> 
> >  		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.

OK, make sense to me. But that means I need to add log(which can be more specific) after every check, do you thinks it's ok?
If you thinks it's ok, then I'll fix it in the next version.

> 
> > +		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.

OK, I'll check these names, and fix them.

Thanks a lot.
Cheng

> 
> Maxime