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.
