> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
>
> On Wed, Sep 25, 2019 at 07:16:23PM +0000, Michal Kalderon wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > > Sent: Friday, September 20, 2019 4:38 PM
> > >
> > > External Email
> > >
> > > 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, by adding the kref to the rdma_user_mmap_entry we sort of
> > disable the option of being sure the entry is removed from the mmap
> > xarray when it is removed by the driver (this will only decrease the
> > refcnt). If we call free_page during the uobject teardown, we can't
> > be sure the entry is removed from the mmap_xa, this could lead to us
> > having an entry in the mmap_xa that points to an invalid page.
>
> I suppose I was expecting that the when the object was no longer to be
> shown to userspace the mmap_xa's were somehow neutralized too so new
> mmaps cannot be established.
Adding/removing entries is dynamic and done on qp / cq creation and destruction,
Could be done all the time. To neutralize them is only to add some interface that
Make sure the entry is deleted like wait for the event that refcnt reaches zero before freeing the memory,
Or leave it as it is now and only free the memory in the mmap_free.
>
> Jason
