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
Jason
