On Fri, Nov 06, 2020 at 01:11:38AM +0000, Xiong, Jianxin wrote: > > On Thu, Nov 05, 2020 at 02:48:08PM -0800, Jianxin Xiong wrote: > > > @@ -966,7 +969,10 @@ static struct mlx5_ib_mr *alloc_mr_from_cache(struct ib_pd *pd, > > > struct mlx5_ib_mr *mr; > > > unsigned int page_size; > > > > > > - page_size = mlx5_umem_find_best_pgsz(umem, mkc, log_page_size, 0, iova); > > > + if (umem->is_dmabuf) > > > + page_size = ib_umem_find_best_pgsz(umem, PAGE_SIZE, iova); > > > > You said the sgl is not set here, why doesn't this crash? It is certainly wrong to call this function without a SGL. > > The sgl is NULL, and nmap is 0. The 'for_each_sg' loop is just skipped and won't crash. Just wire this to 4k it is clearer than calling some no-op pgsz > > > + if (!mr->cache_ent) { > > > + mlx5_core_destroy_mkey(mr->dev->mdev, &mr->mmkey); > > > + WARN_ON(mr->descs); > > > + } > > > +} > > > > I would expect this to call ib_umem_dmabuf_unmap_pages() ? > > > > Who calls it on the dereg path? > > > > This looks quite strange to me, it calls ib_umem_dmabuf_unmap_pages() only from the invalidate callback? > > It is also called from ib_umem_dmabuf_release(). Hmm, that is no how the other APIs work, the unmap should be paired with the map in the caller, and the sequence for destroy should be invalidate unmap destroy_mkey release_umem I have another series coming that makes the other three destroy flows much closer to that ideal. > > I feel uneasy how this seems to assume everything works sanely, we can have parallel page faults so pagefault_dmabuf_mr() can be called > > multiple times after an invalidation, and it doesn't protect itself against calling ib_umem_dmabuf_map_pages() twice. > > > > Perhaps the umem code should keep track of the current map state and exit if there is already a sgl. NULL or not NULL sgl would do and > > seems quite reasonable. > > Ib_umem_dmabuf_map() already checks the sgl and will do nothing if it is already set. How? What I see in patch 1 is an unconditonal call to dma_buf_map_attachment() ? > > > + if (is_dmabuf_mr(mr)) > > > + return pagefault_dmabuf_mr(mr, umem_dmabuf, user_va, > > > + bcnt, bytes_mapped, flags); > > > > But this doesn't care about user_va or bcnt it just triggers the whole thing to be remapped, so why calculate it? > > The range check is still needed, in order to catch application > errors of using incorrect address or count in verbs command. Passing > the values further in is to allow pagefault_dmabuf_mr to generate > return value and set bytes_mapped in a way consistent with the page > fault handler chain. The HW validates the range. The range check in the ODP case is to protect against a HW bug that would cause the kernel to malfunction. For dmabuf you don't need to do it Jason _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel