Re: [PATCH v7 rdma-next 3/7] RDMA/efa: Use the common mmap_xa helpers

[Gal Pressman <galpress@amazon.com>]

On 25/08/2019 11:41, Michal Kalderon wrote:
>>> @@ -515,46 +408,55 @@ static int qp_mmap_entries_setup(struct efa_qp
>> *qp,
>>>  				 struct efa_com_create_qp_params
>> *params,
>>>  				 struct efa_ibv_create_qp_resp *resp)  {
>>> +	u64 address;
>>> +	u64 length;
>>> +
>>>  	/*
>>>  	 * Once an entry is inserted it might be mmapped, hence cannot be
>>>  	 * cleaned up until dealloc_ucontext.
>>>  	 */
>>>  	resp->sq_db_mmap_key =
>>
>> Not a big deal, but now it makes more sense to assign qp-
>>> sq_db_mmap_key and assign the response later on.
> ok
>>
>>> -		mmap_entry_insert(dev, ucontext, qp,
>>> -				  dev->db_bar_addr + resp->sq_db_offset,
>>> -				  PAGE_SIZE, EFA_MMAP_IO_NC);
>>> -	if (resp->sq_db_mmap_key == EFA_MMAP_INVALID)
>>> +		rdma_user_mmap_entry_insert(&ucontext->ibucontext, qp,
>>> +					    dev->db_bar_addr +
>>> +					    resp->sq_db_offset,
>>> +					    PAGE_SIZE, EFA_MMAP_IO_NC);
>>> +	if (resp->sq_db_mmap_key == RDMA_USER_MMAP_INVALID)
>>>  		return -ENOMEM;
>>> -
>>> +	qp->sq_db_mmap_key = resp->sq_db_mmap_key;
>>>  	resp->sq_db_offset &= ~PAGE_MASK;
>>>
>>> +	address = dev->mem_bar_addr + resp->llq_desc_offset;
>>> +	length = PAGE_ALIGN(params->sq_ring_size_in_bytes +
>>> +			    (resp->llq_desc_offset & ~PAGE_MASK));
>>>  	resp->llq_desc_mmap_key =
>>> -		mmap_entry_insert(dev, ucontext, qp,
>>> -				  dev->mem_bar_addr + resp-
>>> llq_desc_offset,
>>> -				  PAGE_ALIGN(params-
>>> sq_ring_size_in_bytes +
>>> -					     (resp->llq_desc_offset &
>> ~PAGE_MASK)),
>>> -				  EFA_MMAP_IO_WC);
>>> -	if (resp->llq_desc_mmap_key == EFA_MMAP_INVALID)
>>> +		rdma_user_mmap_entry_insert(&ucontext->ibucontext, qp,
>>> +					    address,
>>> +					    length,
>>> +					    EFA_MMAP_IO_WC);
>>> +	if (resp->llq_desc_mmap_key == RDMA_USER_MMAP_INVALID)
>>>  		return -ENOMEM;
>>> -
>>> +	qp->llq_desc_mmap_key = resp->llq_desc_mmap_key;
>>>  	resp->llq_desc_offset &= ~PAGE_MASK;
>>>
>>>  	if (qp->rq_size) {
>>> +		address = dev->db_bar_addr + resp->rq_db_offset;
>>>  		resp->rq_db_mmap_key =
>>> -			mmap_entry_insert(dev, ucontext, qp,
>>> -					  dev->db_bar_addr + resp-
>>> rq_db_offset,
>>> -					  PAGE_SIZE, EFA_MMAP_IO_NC);
>>> -		if (resp->rq_db_mmap_key == EFA_MMAP_INVALID)
>>> +			rdma_user_mmap_entry_insert(&ucontext-
>>> ibucontext, qp,
>>> +						    address, PAGE_SIZE,
>>> +						    EFA_MMAP_IO_NC);
>>> +		if (resp->rq_db_mmap_key ==
>> RDMA_USER_MMAP_INVALID)
>>>  			return -ENOMEM;
>>> -
>>> +		qp->rq_db_mmap_key = resp->rq_db_mmap_key;
>>>  		resp->rq_db_offset &= ~PAGE_MASK;
>>>
>>> +		address = virt_to_phys(qp->rq_cpu_addr);
>>>  		resp->rq_mmap_key =
>>> -			mmap_entry_insert(dev, ucontext, qp,
>>> -					  virt_to_phys(qp->rq_cpu_addr),
>>> -					  qp->rq_size,
>> EFA_MMAP_DMA_PAGE);
>>> -		if (resp->rq_mmap_key == EFA_MMAP_INVALID)
>>> +			rdma_user_mmap_entry_insert(&ucontext-
>>> ibucontext, qp,
>>> +						    address, qp->rq_size,
>>> +						    EFA_MMAP_DMA_PAGE);
>>> +		if (resp->rq_mmap_key == RDMA_USER_MMAP_INVALID)
>>>  			return -ENOMEM;
>>> +		qp->rq_mmap_key = resp->rq_mmap_key;
>>>
>>>  		resp->rq_mmap_size = qp->rq_size;
>>>  	}
>>> @@ -775,6 +677,9 @@ struct ib_qp *efa_create_qp(struct ib_pd *ibpd,
>>>  				 DMA_TO_DEVICE);
>>>  		if (!rq_entry_inserted)
>>
>> Now that we store the keys on the QP object we can remove the
>> rq_entry_inserted variable and test for !qp->rq_mmap_key.
> ok
>>
>>>  			free_pages_exact(qp->rq_cpu_addr, qp->rq_size);
>>> +		else
>>> +			rdma_user_mmap_entry_remove(&ucontext-
>>> ibucontext,
>>> +						    qp->rq_mmap_key);
>>
>> Other entries need to be removed as well, otherwise the refcount won't
>> reach zero. This error flow should now be similar to efa_destroy_qp. I think
>> that means losing the free_pages_exact too.
> Not sure I understand, how can we loose the free_pages_exact ? if the entry wasn’t 
> Inserted into the mmap_xa what flow will free the pages ? 

You're right.
Still need to remove other entries though.