Re: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace memory region
On Fri, Nov 13, 2020 at 03:51:20AM +, Xiong, Jianxin wrote: > > > +static void mlx5_ib_dmabuf_invalidate_cb(struct dma_buf_attachment > > > +*attach) { > > > + struct ib_umem_dmabuf *umem_dmabuf = attach->importer_priv; > > > + struct mlx5_ib_mr *mr = umem_dmabuf->private; > > > + > > > + dma_resv_assert_held(umem_dmabuf->attach->dmabuf->resv); > > > + > > > + if (mr) > > > > I don't think this 'if (mr)' test is needed anymore? I certainly prefer it > > gone as it is kind of messy. I expect unmapping the dma to ensure this > > function is not running, and won't run again. > > It is still needed. When the dma-buf moves, the callback function of every > attached importer is invoked, regardless if the importer has mapped the dma > or not. > > > > > > +/** > > > + * mlx5_ib_fence_dmabuf_mr - Stop all access to the dmabuf MR > > > + * @mr: to fence > > > + * > > > + * On return no parallel threads will be touching this MR and no DMA > > > +will be > > > + * active. > > > + */ > > > +void mlx5_ib_fence_dmabuf_mr(struct mlx5_ib_mr *mr) { > > > + struct ib_umem_dmabuf *umem_dmabuf = to_ib_umem_dmabuf(mr->umem); > > > + > > > + /* Prevent new page faults and prefetch requests from succeeding */ > > > + xa_erase(>dev->odp_mkeys, mlx5_base_mkey(mr->mmkey.key)); > > > + > > > + /* Wait for all running page-fault handlers to finish. */ > > > + synchronize_srcu(>dev->odp_srcu); > > > + > > > + wait_event(mr->q_deferred_work, > > > +!atomic_read(>num_deferred_work)); > > > + > > > + dma_resv_lock(umem_dmabuf->attach->dmabuf->resv, NULL); > > > + mlx5_mr_cache_invalidate(mr); > > > + umem_dmabuf->private = NULL; > > > + ib_umem_dmabuf_unmap_pages(umem_dmabuf); > > > + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); > > > + > > > + if (!mr->cache_ent) { > > > + mlx5_core_destroy_mkey(mr->dev->mdev, >mmkey); > > > + WARN_ON(mr->descs); > > > + } > > > > I didn't check carefully, but are you sure this destroy_mkey should be > > here?? > > To my understanding, yes. This is similar to what dma_fence_odp_mr() does, > just inlined here since it's not called from other places. I think you should put the calls to dma_buf_dynamic_attach() and dma_buf_detach() into mlx5, it makes the whole thing a little cleaner, then the umem->private isn't needed any more either. Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace memory region
> -Original Message- > From: Jason Gunthorpe > Sent: Friday, November 13, 2020 5:06 AM > To: Xiong, Jianxin > Cc: linux-r...@vger.kernel.org; dri-devel@lists.freedesktop.org; Doug Ledford > ; Leon Romanovsky > ; Sumit Semwal ; Christian Koenig > ; Vetter, Daniel > > Subject: Re: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace > memory region > > On Fri, Nov 13, 2020 at 03:51:20AM +, Xiong, Jianxin wrote: > > > > > +static void mlx5_ib_dmabuf_invalidate_cb(struct > > > > +dma_buf_attachment > > > > +*attach) { > > > > + struct ib_umem_dmabuf *umem_dmabuf = attach->importer_priv; > > > > + struct mlx5_ib_mr *mr = umem_dmabuf->private; > > > > + > > > > + dma_resv_assert_held(umem_dmabuf->attach->dmabuf->resv); > > > > + > > > > + if (mr) > > > > > > I don't think this 'if (mr)' test is needed anymore? I certainly > > > prefer it gone as it is kind of messy. I expect unmapping the dma to > > > ensure this function is not running, and won't run again. > > > > It is still needed. When the dma-buf moves, the callback function of every > > attached importer is invoked, regardless if the importer has > mapped the dma or not. > > > > > > > > > +/** > > > > + * mlx5_ib_fence_dmabuf_mr - Stop all access to the dmabuf MR > > > > + * @mr: to fence > > > > + * > > > > + * On return no parallel threads will be touching this MR and no > > > > +DMA will be > > > > + * active. > > > > + */ > > > > +void mlx5_ib_fence_dmabuf_mr(struct mlx5_ib_mr *mr) { > > > > + struct ib_umem_dmabuf *umem_dmabuf = > > > > +to_ib_umem_dmabuf(mr->umem); > > > > + > > > > + /* Prevent new page faults and prefetch requests from > > > > succeeding */ > > > > + xa_erase(>dev->odp_mkeys, mlx5_base_mkey(mr->mmkey.key)); > > > > + > > > > + /* Wait for all running page-fault handlers to finish. */ > > > > + synchronize_srcu(>dev->odp_srcu); > > > > + > > > > + wait_event(mr->q_deferred_work, > > > > +!atomic_read(>num_deferred_work)); > > > > + > > > > + dma_resv_lock(umem_dmabuf->attach->dmabuf->resv, NULL); > > > > + mlx5_mr_cache_invalidate(mr); > > > > + umem_dmabuf->private = NULL; > > > > + ib_umem_dmabuf_unmap_pages(umem_dmabuf); > > > > + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); > > > > + > > > > + if (!mr->cache_ent) { > > > > + mlx5_core_destroy_mkey(mr->dev->mdev, >mmkey); > > > > + WARN_ON(mr->descs); > > > > + } > > > > > > I didn't check carefully, but are you sure this destroy_mkey should be > > > here?? > > > > To my understanding, yes. This is similar to what dma_fence_odp_mr() > > does, just inlined here since it's not called from other places. > > I think you should put the calls to dma_buf_dynamic_attach() and > dma_buf_detach() into mlx5, it makes the whole thing a little cleaner, then > the umem->private isn't needed any more either. Putting these calls into mlx5 can remove the 'ops' parameter from the umem_get call, but I don't see how umem->private can be eliminated. In addition, I feel keeping these two calls in the core provides a better separation between the core and the driver -- dma-buf API functions are only called from the core, except for locking/unlocking. The 'if (mr)' part in the callback can be removed by adding a line 'if (!umem_dmabutf->sgt) return;' before that if that makes a difference. > > Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace memory region
On Tue, Nov 10, 2020 at 01:41:15PM -0800, Jianxin Xiong wrote: > -static int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int flags) > +int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int flags) > { > struct mlx5_ib_dev *dev = mr->dev; > struct device *ddev = dev->ib_dev.dev.parent; > @@ -1255,6 +1267,10 @@ static int mlx5_ib_update_mr_pas(struct mlx5_ib_mr > *mr, unsigned int flags) > cur_mtt->ptag = > cpu_to_be64(rdma_block_iter_dma_address() | > MLX5_IB_MTT_PRESENT); > + > + if (mr->umem->is_dmabuf && (flags & MLX5_IB_UPD_XLT_ZAP)) > + cur_mtt->ptag = 0; > + > cur_mtt++; > } > > @@ -1291,8 +1307,15 @@ static struct mlx5_ib_mr *reg_create(struct ib_mr > *ibmr, struct ib_pd *pd, > int err; > bool pg_cap = !!(MLX5_CAP_GEN(dev->mdev, pg)); > > - page_size = > - mlx5_umem_find_best_pgsz(umem, mkc, log_page_size, 0, iova); > + if (umem->is_dmabuf) { > + if ((iova ^ umem->address) & (PAGE_SIZE - 1)) > + return ERR_PTR(-EINVAL); > + umem->iova = iova; > + page_size = PAGE_SIZE; > + } else { > + page_size = mlx5_umem_find_best_pgsz(umem, mkc, log_page_size, > + 0, iova); > + } Urk, maybe this duplicated sequence should be in a function.. This will also collide with a rereg_mr bugfixing series that should be posted soon.. > +static void mlx5_ib_dmabuf_invalidate_cb(struct dma_buf_attachment *attach) > +{ > + struct ib_umem_dmabuf *umem_dmabuf = attach->importer_priv; > + struct mlx5_ib_mr *mr = umem_dmabuf->private; > + > + dma_resv_assert_held(umem_dmabuf->attach->dmabuf->resv); > + > + if (mr) I don't think this 'if (mr)' test is needed anymore? I certainly prefer it gone as it is kind of messy. I expect unmapping the dma to ensure this function is not running, and won't run again. > +/** > + * mlx5_ib_fence_dmabuf_mr - Stop all access to the dmabuf MR > + * @mr: to fence > + * > + * On return no parallel threads will be touching this MR and no DMA will be > + * active. > + */ > +void mlx5_ib_fence_dmabuf_mr(struct mlx5_ib_mr *mr) > +{ > + struct ib_umem_dmabuf *umem_dmabuf = to_ib_umem_dmabuf(mr->umem); > + > + /* Prevent new page faults and prefetch requests from succeeding */ > + xa_erase(>dev->odp_mkeys, mlx5_base_mkey(mr->mmkey.key)); > + > + /* Wait for all running page-fault handlers to finish. */ > + synchronize_srcu(>dev->odp_srcu); > + > + wait_event(mr->q_deferred_work, !atomic_read(>num_deferred_work)); > + > + dma_resv_lock(umem_dmabuf->attach->dmabuf->resv, NULL); > + mlx5_mr_cache_invalidate(mr); > + umem_dmabuf->private = NULL; > + ib_umem_dmabuf_unmap_pages(umem_dmabuf); > + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); > + > + if (!mr->cache_ent) { > + mlx5_core_destroy_mkey(mr->dev->mdev, >mmkey); > + WARN_ON(mr->descs); > + } I didn't check carefully, but are you sure this destroy_mkey should be here?? Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace memory region
> -Original Message- > From: Jason Gunthorpe > Sent: Thursday, November 12, 2020 4:40 PM > To: Xiong, Jianxin > Cc: linux-r...@vger.kernel.org; dri-devel@lists.freedesktop.org; Doug Ledford > ; Leon Romanovsky > ; Sumit Semwal ; Christian Koenig > ; Vetter, Daniel > > Subject: Re: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace > memory region > > On Tue, Nov 10, 2020 at 01:41:15PM -0800, Jianxin Xiong wrote: > > > -static int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int > > flags) > > +int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int flags) > > { > > struct mlx5_ib_dev *dev = mr->dev; > > struct device *ddev = dev->ib_dev.dev.parent; @@ -1255,6 +1267,10 @@ > > static int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int flags) > > cur_mtt->ptag = > > cpu_to_be64(rdma_block_iter_dma_address() | > > MLX5_IB_MTT_PRESENT); > > + > > + if (mr->umem->is_dmabuf && (flags & MLX5_IB_UPD_XLT_ZAP)) > > + cur_mtt->ptag = 0; > > + > > cur_mtt++; > > } > > > > @@ -1291,8 +1307,15 @@ static struct mlx5_ib_mr *reg_create(struct ib_mr > > *ibmr, struct ib_pd *pd, > > int err; > > bool pg_cap = !!(MLX5_CAP_GEN(dev->mdev, pg)); > > > > - page_size = > > - mlx5_umem_find_best_pgsz(umem, mkc, log_page_size, 0, iova); > > + if (umem->is_dmabuf) { > > + if ((iova ^ umem->address) & (PAGE_SIZE - 1)) > > + return ERR_PTR(-EINVAL); > > + umem->iova = iova; > > + page_size = PAGE_SIZE; > > + } else { > > + page_size = mlx5_umem_find_best_pgsz(umem, mkc, log_page_size, > > +0, iova); > > + } > > Urk, maybe this duplicated sequence should be in a function.. > > This will also collide with a rereg_mr bugfixing series that should be posted > soon.. > > > +static void mlx5_ib_dmabuf_invalidate_cb(struct dma_buf_attachment > > +*attach) { > > + struct ib_umem_dmabuf *umem_dmabuf = attach->importer_priv; > > + struct mlx5_ib_mr *mr = umem_dmabuf->private; > > + > > + dma_resv_assert_held(umem_dmabuf->attach->dmabuf->resv); > > + > > + if (mr) > > I don't think this 'if (mr)' test is needed anymore? I certainly prefer it > gone as it is kind of messy. I expect unmapping the dma to ensure this > function is not running, and won't run again. It is still needed. When the dma-buf moves, the callback function of every attached importer is invoked, regardless if the importer has mapped the dma or not. > > > +/** > > + * mlx5_ib_fence_dmabuf_mr - Stop all access to the dmabuf MR > > + * @mr: to fence > > + * > > + * On return no parallel threads will be touching this MR and no DMA > > +will be > > + * active. > > + */ > > +void mlx5_ib_fence_dmabuf_mr(struct mlx5_ib_mr *mr) { > > + struct ib_umem_dmabuf *umem_dmabuf = to_ib_umem_dmabuf(mr->umem); > > + > > + /* Prevent new page faults and prefetch requests from succeeding */ > > + xa_erase(>dev->odp_mkeys, mlx5_base_mkey(mr->mmkey.key)); > > + > > + /* Wait for all running page-fault handlers to finish. */ > > + synchronize_srcu(>dev->odp_srcu); > > + > > + wait_event(mr->q_deferred_work, > > +!atomic_read(>num_deferred_work)); > > + > > + dma_resv_lock(umem_dmabuf->attach->dmabuf->resv, NULL); > > + mlx5_mr_cache_invalidate(mr); > > + umem_dmabuf->private = NULL; > > + ib_umem_dmabuf_unmap_pages(umem_dmabuf); > > + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); > > + > > + if (!mr->cache_ent) { > > + mlx5_core_destroy_mkey(mr->dev->mdev, >mmkey); > > + WARN_ON(mr->descs); > > + } > > I didn't check carefully, but are you sure this destroy_mkey should be here?? To my understanding, yes. This is similar to what dma_fence_odp_mr() does, just inlined here since it's not called from other places. > > Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel