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

[Gal Pressman <galpress@amazon.com>]

On 20/08/2019 15:18, Michal Kalderon wrote:
>  int efa_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
>  {
> +	struct efa_ucontext *ucontext;

Reverse xmas tree please.

>  	struct efa_dev *dev = to_edev(ibqp->pd->device);
>  	struct efa_qp *qp = to_eqp(ibqp);
>  	int err;
>  
>  	ibdev_dbg(&dev->ibdev, "Destroy qp[%u]\n", ibqp->qp_num);
> +	ucontext = rdma_udata_to_drv_context(udata, struct efa_ucontext,
> +					     ibucontext);

Consider initializing on ucontext declaration.

> +
>  	err = efa_destroy_qp_handle(dev, qp->qp_handle);
>  	if (err)
>  		return err;
>  
> +	rdma_user_mmap_entry_remove(&ucontext->ibucontext, qp->sq_db_mmap_key);
> +	rdma_user_mmap_entry_remove(&ucontext->ibucontext,
> +				    qp->llq_desc_mmap_key);

The mmap entries removal should happen before efa_destroy_qp_handle.

>  	if (qp->rq_cpu_addr) {
>  		ibdev_dbg(&dev->ibdev,
>  			  "qp->cpu_addr[0x%p] freed: size[%lu], dma[%pad]\n",
> @@ -503,6 +392,10 @@ int efa_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
>  			  &qp->rq_dma_addr);
>  		dma_unmap_single(&dev->pdev->dev, qp->rq_dma_addr, qp->rq_size,
>  				 DMA_TO_DEVICE);
> +		rdma_user_mmap_entry_remove(&ucontext->ibucontext,
> +					    qp->rq_mmap_key);
> +		rdma_user_mmap_entry_remove(&ucontext->ibucontext,
> +					    qp->rq_db_mmap_key);

Same.

>  	}
>  
>  	kfree(qp);
> @@ -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.

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

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

>  	}
>  err_free_qp:
>  	kfree(qp);

Pretty much the same comments for the CQ parts as the QP.