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

[Xiong, Jianxin]

> -----Original Message-----
> From: Jason Gunthorpe <jgg at ziepe.ca>
> Sent: Friday, November 13, 2020 5:06 AM
> To: Xiong, Jianxin <jianxin.xiong at intel.com>
> Cc: linux-rdma at vger.kernel.org; dri-devel at lists.freedesktop.org; Doug Ledford <dledford at redhat.com>; Leon Romanovsky
> <leon at kernel.org>; Sumit Semwal <sumit.semwal at linaro.org>; Christian Koenig <christian.koenig at amd.com>; Vetter, Daniel
> <daniel.vetter at intel.com>
> Subject: Re: [PATCH v10 4/6] RDMA/mlx5: Support dma-buf based userspace memory region
> 
> On Fri, Nov 13, 2020 at 03:51:20AM +0000, 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(&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.
> 
> 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