Re: [PATCH v11 rdma-next 5/7] RDMA/qedr: Use the common mmap API

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Michal Kalderon <michal.kalderon@marvell.com>
Cc: mkalderon@marvell.com, aelior@marvell.com, dledford@redhat.com,
	bmt@zurich.ibm.com, galpress@amazon.com, sleybo@amazon.com,
	leon@kernel.org, linux-rdma@vger.kernel.org
Subject: Re: [PATCH v11 rdma-next 5/7] RDMA/qedr: Use the common mmap API
Date: Thu, 19 Sep 2019 14:55:46 -0300	[thread overview]
Message-ID: <20190919175546.GD4132@ziepe.ca> (raw)
In-Reply-To: <20190905100117.20879-6-michal.kalderon@marvell.com>

On Thu, Sep 05, 2019 at 01:01:15PM +0300, Michal Kalderon wrote:

> -int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> +void qedr_mmap_free(struct rdma_user_mmap_entry *rdma_entry)
>  {
> -	struct qedr_ucontext *ucontext = get_qedr_ucontext(context);
> -	struct qedr_dev *dev = get_qedr_dev(context->device);
> -	unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
> -	unsigned long len = (vma->vm_end - vma->vm_start);
> -	unsigned long dpi_start;
> +	struct qedr_user_mmap_entry *entry = get_qedr_mmap_entry(rdma_entry);
>  
> -	dpi_start = dev->db_phys_addr + (ucontext->dpi * ucontext->dpi_size);
> -
> -	DP_DEBUG(dev, QEDR_MSG_INIT,
> -		 "mmap invoked with vm_start=0x%pK, vm_end=0x%pK,vm_pgoff=0x%pK; dpi_start=0x%pK dpi_size=0x%x\n",
> -		 (void *)vma->vm_start, (void *)vma->vm_end,
> -		 (void *)vma->vm_pgoff, (void *)dpi_start, ucontext->dpi_size);
> +	kfree(entry);
> +}

Huh. If you recall we did all this work with the XA and the free
callback because you said qedr was mmaping BAR pages that had some HW
lifetime associated with them, and the HW resource was not to be
reallocated until all users were gone.

I think it would be a better example of this API if you pulled the

 	dev->ops->rdma_remove_user(dev->rdma_ctx, ctx->dpi);

Into qedr_mmap_free().

Then the rdma_user_mmap_entry_remove() will call it naturally as it
does entry_put() and if we are destroying the ucontext we already know
the mmaps are destroyed.

Maybe the same basic comment for EFA, not sure. Gal?

Jason