On 15/01/2020 21:51, Jason Gunthorpe wrote: > On Tue, Jan 14, 2020 at 10:57:06AM +0200, Gal Pressman wrote: >> When destroying a DMA mmapped object, there is no need to delay the >> pages freeing to dealloc_ucontext as the kernel itself will keep >> reference count for these pages. > > Why does the commit message talk about dealloc_ucontext but doesn't > change dealloc_ucontext? The commit message is wrong :\, we should not delay the free_pages_exact to the mmap_free callback. We can "put" them on destroy flow and the pages will be freed by the last consumer (either unmap or destroy). > >> + free_pages_exact(cq->cpu_addr, cq->size); >> rdma_user_mmap_entry_remove(cq->mmap_entry); > > This is out of order, the pages can't be freed until the entry is > removed. Right, I think the order is correct except rdma_user_mmap_entry_remove should be moved to the beginning of the function. > > There is also a bug in rdma_user_mmap_entry_remove(), > entry->driver_removed needs to be set while holding the xa_lock or > this is not the required fence. I see you sent a patch, I'll take a look. Thanks for the review.
