Re: [dpdk-dev] [RFC 01/14] vhost: move async data in a dedicated structure

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

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, October 8, 2021 6:00 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Wang, YuanX <yuanx.wang@intel.com>; Ma,
> WenwuX <wenwux.ma@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [RFC 01/14] vhost: move async data in a dedicated structure
> 
> This patch moves async-related metadata from vhost_virtqueue to a
> dedicated struct. It makes it clear which fields are async related, and also
> saves some memory when async feature is not in use.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vhost.c      | 129 ++++++++++++++++-------------------------
>  lib/vhost/vhost.h      |  53 ++++++++---------
>  lib/vhost/vhost_user.c |   4 +-
>  lib/vhost/virtio_net.c | 114 +++++++++++++++++++-----------------
>  4 files changed, 140 insertions(+), 160 deletions(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
> 9540522dac..58f72b633c 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -340,19 +340,15 @@ cleanup_device(struct virtio_net *dev, int destroy)
> static void  vhost_free_async_mem(struct vhost_virtqueue *vq)  {
> -	rte_free(vq->async_pkts_info);
> +	rte_free(vq->async->pkts_info);

Apps may unregister async in vring_state_changed() explicitly when
vring is disabled. In this case, rte_vhost_async_channel_unregister()
will call vhost_free_async_mem() first, so that vq->async becomes NULL.
But after then device is destroyed, free_vq() calls vhost_free_async_mem()
again. "rte_free(vq->async->pkts_info)" will try to read a NULL pointer,
which will cause segment fault.

> 
> -	rte_free(vq->async_buffers_packed);
> -	vq->async_buffers_packed = NULL;
> -	rte_free(vq->async_descs_split);
> -	vq->async_descs_split = NULL;
> +	rte_free(vq->async->buffers_packed);
> +	vq->async->buffers_packed = NULL;
> +	rte_free(vq->async->descs_split);
> +	vq->async->descs_split = NULL;
> 
> -	rte_free(vq->it_pool);
> -	rte_free(vq->vec_pool);
> -
> -	vq->async_pkts_info = NULL;
> -	vq->it_pool = NULL;
> -	vq->vec_pool = NULL;
> +	rte_free(vq->async);
> +	vq->async = NULL;
>  }
> 
>  void
> @@ -1629,77 +1625,63 @@ async_channel_register(int vid, uint16_t
> queue_id,  {
>  	struct virtio_net *dev = get_device(vid);
>  	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
> +	struct vhost_async *async;
> +	int node = vq->numa_node;
> 
> -	if (unlikely(vq->async_registered)) {
> +	if (unlikely(vq->async)) {
>  		VHOST_LOG_CONFIG(ERR,
> -			"async register failed: channel already registered "
> -			"(vid %d, qid: %d)\n", vid, queue_id);
> +				"async register failed: already registered
> (vid %d, qid: %d)\n",
> +				vid, queue_id);
>  		return -1;
>  	}
> 
> -	vq->async_pkts_info = rte_malloc_socket(NULL,
> -			vq->size * sizeof(struct async_inflight_info),
> -			RTE_CACHE_LINE_SIZE, vq->numa_node);
> -	if (!vq->async_pkts_info) {
> -		vhost_free_async_mem(vq);
> -		VHOST_LOG_CONFIG(ERR,
> -			"async register failed: cannot allocate memory for
> async_pkts_info "
> -			"(vid %d, qid: %d)\n", vid, queue_id);
> +	async = rte_zmalloc_socket(NULL, sizeof(struct vhost_async), 0,
> node);
> +	if (!async) {
> +		VHOST_LOG_CONFIG(ERR, "failed to allocate async metadata
> (vid %d, qid: %d)\n",
> +				vid, queue_id);
>  		return -1;
>  	}
> 
> -	vq->it_pool = rte_malloc_socket(NULL,
> -			VHOST_MAX_ASYNC_IT * sizeof(struct
> rte_vhost_iov_iter),
> -			RTE_CACHE_LINE_SIZE, vq->numa_node);
> -	if (!vq->it_pool) {
> -		vhost_free_async_mem(vq);
> -		VHOST_LOG_CONFIG(ERR,
> -			"async register failed: cannot allocate memory for
> it_pool "
> -			"(vid %d, qid: %d)\n", vid, queue_id);
> -		return -1;
> -	}
> -
> -	vq->vec_pool = rte_malloc_socket(NULL,
> -			VHOST_MAX_ASYNC_VEC * sizeof(struct iovec),
> -			RTE_CACHE_LINE_SIZE, vq->numa_node);
> -	if (!vq->vec_pool) {
> -		vhost_free_async_mem(vq);
> -		VHOST_LOG_CONFIG(ERR,
> -			"async register failed: cannot allocate memory for
> vec_pool "
> -			"(vid %d, qid: %d)\n", vid, queue_id);
> -		return -1;
> +	async->pkts_info = rte_malloc_socket(NULL, vq->size * sizeof(struct
> async_inflight_info),
> +			RTE_CACHE_LINE_SIZE, node);
> +	if (async->pkts_info) {

It should be "if (!async->pkts_info)".

Thanks,
Jiayu