RE: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace memory region

["Xiong, Jianxin" <jianxin.xiong@intel.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, November 12, 2020 4:40 PM
> To: Xiong, Jianxin <jianxin.xiong@intel.com>
> Cc: linux-rdma@vger.kernel.org; dri-devel@lists.freedesktop.org; Doug Ledford <dledford@redhat.com>; Leon Romanovsky
> <leon@kernel.org>; Sumit Semwal <sumit.semwal@linaro.org>; Christian Koenig <christian.koenig@amd.com>; Vetter, Daniel
> <daniel.vetter@intel.com>
> 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(&biter) |
> >  				    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(&mr->dev->odp_mkeys, mlx5_base_mkey(mr->mmkey.key));
> > +
> > +	/* Wait for all running page-fault handlers to finish. */
> > +	synchronize_srcu(&mr->dev->odp_srcu);
> > +
> > +	wait_event(mr->q_deferred_work,
> > +!atomic_read(&mr->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, &mr->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