Re: [PATCH v11 rdma-next 6/7] RDMA/qedr: Add doorbell overflow recovery support

From: Gal Pressman <galpress@amazon.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Michal Kalderon <michal.kalderon@marvell.com>,
	<mkalderon@marvell.com>, <aelior@marvell.com>,
	<dledford@redhat.com>, <bmt@zurich.ibm.com>, <sleybo@amazon.com>,
	<leon@kernel.org>, <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH v11 rdma-next 6/7] RDMA/qedr: Add doorbell overflow recovery support
Date: Fri, 20 Sep 2019 17:00:25 +0300	[thread overview]
Message-ID: <82541758-3e72-d485-6923-cf3336a4297a@amazon.com> (raw)
In-Reply-To: <20190920133817.GB7095@ziepe.ca>

On 20/09/2019 16:38, Jason Gunthorpe wrote:
> On Fri, Sep 20, 2019 at 04:30:52PM +0300, Gal Pressman wrote:
>> On 19/09/2019 21:02, Jason Gunthorpe wrote:
>>> On Thu, Sep 05, 2019 at 01:01:16PM +0300, Michal Kalderon wrote:
>>>
>>>> @@ -347,6 +360,9 @@ void qedr_mmap_free(struct rdma_user_mmap_entry *rdma_entry)
>>>>  {
>>>>  	struct qedr_user_mmap_entry *entry = get_qedr_mmap_entry(rdma_entry);
>>>>  
>>>> +	if (entry->mmap_flag == QEDR_USER_MMAP_PHYS_PAGE)
>>>> +		free_page((unsigned long)phys_to_virt(entry->address));
>>>> +
>>>
>>> While it isn't wrong it do it this way, we don't need this mmap_free()
>>> stuff for normal CPU pages. Those are refcounted and qedr can simply
>>> call free_page() during the teardown of the uobject that is using the
>>> this page. This is what other drivers already do.
>>
>> This is pretty much what EFA does as well.  When we allocate pages
>> for the user (CQ for example), we DMA map them and later on mmap
>> them to the user. We expect those pages to remain until the entry is
>> freed, how can we call free_page, who is holding a refcount on those
>> except for the driver?
> 
> free_page is kind of a lie, it is more like put_page (see
> __free_pages). I think the difference is that it assumes the page came
> from alloc_page and skips some generic stuff when freeing it.
> 
> When the driver does vm_insert_page the vma holds another refcount and
> the refcount does not go to zero until that page drops out of the
> vma (ie at the same time mmap_free above is called). 
> 
> Then __put_page will do the free_unref_page(), etc.
> 
> So for CPU pages it is fine to not use mmap_free so long as
> vm_insert_page is used

Thanks, I did not know this, it simplifies things.
In that case, maybe the mmap_free callback is redundant.