On Thu, Nov 05, 2020 at 12:36:25AM +0000, Xiong, Jianxin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > Sent: Wednesday, November 04, 2020 4:07 PM
> > To: Xiong, Jianxin <jianxin.xiong@xxxxxxxxx>
> > Cc: linux-rdma@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> > <leon@xxxxxxxxxx>; Sumit Semwal <sumit.semwal@xxxxxxxxxx>; Christian Koenig <christian.koenig@xxxxxxx>; Vetter, Daniel
> > <daniel.vetter@xxxxxxxxx>
> > 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
