Re: [PATCH v7 4/5] RDMA/mlx5: Support dma-buf based userspace memory region

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Nov 05, 2020 at 12:36:25AM +0000, Xiong, Jianxin wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Wednesday, November 04, 2020 4:07 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 v7 4/5] RDMA/mlx5: Support dma-buf based userspace memory region
> > 
> > On Wed, Nov 04, 2020 at 02:06:34PM -0800, Jianxin Xiong wrote:
> > > +	umem = ib_umem_dmabuf_get(&dev->ib_dev, offset, length, fd, access_flags,
> > > +				  &mlx5_ib_dmabuf_attach_ops);
> > > +	if (IS_ERR(umem)) {
> > > +		mlx5_ib_dbg(dev, "umem get failed (%ld)\n", PTR_ERR(umem));
> > > +		return ERR_PTR(PTR_ERR(umem));
> > > +	}
> > > +
> > > +	mr = alloc_mr_from_cache(pd, umem, virt_addr, access_flags);
> > 
> > It is very subtle, but this calls mlx5_umem_find_best_pgsz() which calls ib_umem_find_best_pgsz() which goes over the SGL to determine
> > the page size to use.
> > 
> 
> When this is called here, the umem sglist is still NULL because dma_buf_map_attachment()
> is not called until a page fault occurs. In patch 1/5, the function ib_umem_find_best_pgsz()
> has been modified to always return PAGE_SIZE for dma-buf based MR.

Oh.. That isn't a good idea.

ib_umem_find_best_pgsz() must be run on any SGL list to validate it
against the constraints, making it un-runable for the dmabuf case
means we can never support large page size or even validate that the
SGL is properly formed.

So I think this need to change the alloc_mr_from_cache() to early exit
for dma_buf ones

And it still need to call ib_umem_find_best_pgsz() but
just check the page size.

> > Edit the last SGE to have a reduced length
> 
> Do you still think modifying the SGL in place needed given the above
> explanation? I do see some benefits of doing so -- hiding the
> discrepancy of sgl and addr/length from the device drivers and avoid
> special handling in the code that use the sgl.

Yes, a umem SGL should always be properly formed or I will have a
meltdown trying to keep all the drivers working :\

Jason