Re: [dpdk-dev] [PATCH 2/2] vhost: add thread unsafe async registration functions

["Hu, Jiayu" <jiayu.hu@intel.com>]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, July 5, 2021 4:59 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Subject: Re: [PATCH 2/2] vhost: add thread unsafe async registration
> functions
> On 5/28/21 10:11 AM, Jiayu Hu wrote:
> > This patch is to add thread unsafe version for async register and
> > unregister functions.
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  doc/guides/prog_guide/vhost_lib.rst |  12 +++
> >  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
> >  lib/vhost/version.map               |   4 +
> >  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
> >  4 files changed, 178 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
> > c96f633..025e150 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1609,46 +1609,20 @@ int rte_vhost_extern_callback_register(int vid,
> >  	return 0;
> >  }
> >
> > -int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > -					uint32_t features,
> > -					struct rte_vhost_async_channel_ops
> *ops)
> > +static __rte_always_inline int
> > +async_channel_register(int vid, uint16_t queue_id,
> > +		struct rte_vhost_async_features f,
> > +		struct rte_vhost_async_channel_ops *ops)
> >  {
> > -	struct vhost_virtqueue *vq;
> >  	struct virtio_net *dev = get_device(vid);
> > -	struct rte_vhost_async_features f;
> > +	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
> >  	int node;
> >
> > -	if (dev == NULL || ops == NULL)
> > -		return -1;
> > -
> > -	f.intval = features;
> > -
> > -	if (queue_id >= VHOST_MAX_VRING)
> > -		return -1;
> > -
> > -	vq = dev->virtqueue[queue_id];
> > -
> > -	if (unlikely(vq == NULL || !dev->async_copy))
> > -		return -1;
> > -
> > -	if (unlikely(!f.async_inorder)) {
> > -		VHOST_LOG_CONFIG(ERR,
> > -			"async copy is not supported on non-inorder mode "
> > -			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		return -1;
> > -	}
> > -
> > -	if (unlikely(ops->check_completed_copies == NULL ||
> > -		ops->transfer_data == NULL))
> > -		return -1;
> > -
> > -	rte_spinlock_lock(&vq->access_lock);
> > -
> >  	if (unlikely(vq->async_registered)) {
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: channel already registered "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  #ifdef RTE_LIBRTE_VHOST_NUMA
> > @@ -1670,7 +1644,7 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: cannot allocate memory for
> async_pkts_info "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  	vq->it_pool = rte_malloc_socket(NULL, @@ -1681,7 +1655,7 @@ int
> > rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: cannot allocate memory for
> it_pool "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  	vq->vec_pool = rte_malloc_socket(NULL, @@ -1692,7 +1666,7 @@
> int
> > rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: cannot allocate memory for
> vec_pool "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  	if (vq_is_packed(dev)) {
> > @@ -1704,7 +1678,7 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  			VHOST_LOG_CONFIG(ERR,
> >  				"async register failed: cannot allocate
> memory for async buffers "
> >  				"(vid %d, qid: %d)\n", vid, queue_id);
> > -			goto reg_out;
> > +			return -1;
> >  		}
> >  	} else {
> >  		vq->async_descs_split = rte_malloc_socket(NULL, @@ -
> 1715,22
> > +1689,92 @@ int rte_vhost_async_channel_register(int vid, uint16_t
> queue_id,
> >  			VHOST_LOG_CONFIG(ERR,
> >  				"async register failed: cannot allocate
> memory for async descs "
> >  				"(vid %d, qid: %d)\n", vid, queue_id);
> > -			goto reg_out;
> > +			return -1;
> >  		}
> >  	}
> >
> >  	vq->async_ops.check_completed_copies = ops-
> >check_completed_copies;
> >  	vq->async_ops.transfer_data = ops->transfer_data;
> > -
> >  	vq->async_inorder = f.async_inorder;
> >  	vq->async_threshold = f.async_threshold;
> > -
> >  	vq->async_registered = true;
> >
> > -reg_out:
> > +	return 0;
> > +}
> > +
> > +int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > +					uint32_t features,
> > +					struct rte_vhost_async_channel_ops
> *ops) {
> > +	struct vhost_virtqueue *vq;
> > +	struct virtio_net *dev = get_device(vid);
> > +	struct rte_vhost_async_features f;
> > +	int ret;
> > +
> > +	if (dev == NULL || ops == NULL)
> > +		return -1;
> > +
> > +	f.intval = features;
> 
> Not directly related to this patch set, but could you please rework struct
> rte_vhost_async_features? There is no point to pack the flags and threshold
> values.
> 
> Also, the prototype should just pass the struct directly, or add different fields
> for the threshold and the features.
> 

Will rework the structure in v2.

Thanks,
Jiayu