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

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

On 10/14/21 05:24, Hu, Jiayu wrote:
> 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.

Good catch, I'll add a check of vq->async before dereferencing it.

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

Correct.

You can notice I didn't lie when I said it was only compile-tested in
the cover-letter! :)

Regards,
Maxime

> Thanks,
> Jiayu
>