Re: [RFC PATCH 2/2] RDMA/efa: Add support for dmabuf memory regions
On 12/10/2021 2:28, Jason Gunthorpe wrote: > On Sun, Oct 10, 2021 at 09:55:49AM +0300, Gal Pressman wrote: >> On 07/10/2021 14:40, Jason Gunthorpe wrote: >>> On Thu, Oct 07, 2021 at 01:43:00PM +0300, Gal Pressman wrote: >>> @@ -1491,26 +1493,29 @@ static int efa_create_pbl(struct efa_dev *dev, return 0; } -struct ib_mr *efa_reg_mr(struct ib_pd *ibpd, u64 start, u64 length, - u64 virt_addr, int access_flags, - struct ib_udata *udata) +static void efa_dmabuf_invalidate_cb(struct dma_buf_attachment *attach) +{ + WARN_ON_ONCE(1, + "Invalidate callback should not be called when memory is pinned\n"); +} + +static struct dma_buf_attach_ops efa_dmabuf_attach_ops = { + .allow_peer2peer = true, + .move_notify = efa_dmabuf_invalidate_cb, +}; >>> >>> Shouldn't move_notify really just be left as NULL? I mean fixing >>> whatever is preventing that? >> >> That's what I had in the previous RFC and I think Christian didn't really >> like it. > > Well, having drivers define a dummy function that only fails looks > a lot worse to me. If not null then it should be a general > 'dmabuf_unsupported_move_notify' shared function Will do. + err = ib_umem_dmabuf_map_pages(umem_dmabuf); + if (err) { + ibdev_dbg(>ibdev, "Failed to map dmabuf pages\n"); + goto err_unpin; + } + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); >>> >>> If it is really this simple the core code should have this logic, >>> 'ib_umem_dmabuf_get_pinned()' or something >> >> Should get_pinned do just get + dma_buf_pin, or should it do >> ib_umem_dmabuf_map_pages as well? > > Yes the map_pages too, a umem is supposed to be dma mapped after > creation. Will do, thanks Jason.
Re: [RFC PATCH 2/2] RDMA/efa: Add support for dmabuf memory regions
On Sun, Oct 10, 2021 at 09:55:49AM +0300, Gal Pressman wrote: > On 07/10/2021 14:40, Jason Gunthorpe wrote: > > On Thu, Oct 07, 2021 at 01:43:00PM +0300, Gal Pressman wrote: > > > >> @@ -1491,26 +1493,29 @@ static int efa_create_pbl(struct efa_dev *dev, > >>return 0; > >> } > >> > >> -struct ib_mr *efa_reg_mr(struct ib_pd *ibpd, u64 start, u64 length, > >> - u64 virt_addr, int access_flags, > >> - struct ib_udata *udata) > >> +static void efa_dmabuf_invalidate_cb(struct dma_buf_attachment *attach) > >> +{ > >> + WARN_ON_ONCE(1, > >> + "Invalidate callback should not be called when memory is > >> pinned\n"); > >> +} > >> + > >> +static struct dma_buf_attach_ops efa_dmabuf_attach_ops = { > >> + .allow_peer2peer = true, > >> + .move_notify = efa_dmabuf_invalidate_cb, > >> +}; > > > > Shouldn't move_notify really just be left as NULL? I mean fixing > > whatever is preventing that? > > That's what I had in the previous RFC and I think Christian didn't really > like it. Well, having drivers define a dummy function that only fails looks a lot worse to me. If not null then it should be a general 'dmabuf_unsupported_move_notify' shared function > >> + err = ib_umem_dmabuf_map_pages(umem_dmabuf); > >> + if (err) { > >> + ibdev_dbg(>ibdev, "Failed to map dmabuf pages\n"); > >> + goto err_unpin; > >> + } > >> + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); > > > > If it is really this simple the core code should have this logic, > > 'ib_umem_dmabuf_get_pinned()' or something > > Should get_pinned do just get + dma_buf_pin, or should it do > ib_umem_dmabuf_map_pages as well? Yes the map_pages too, a umem is supposed to be dma mapped after creation. Jason
Re: [RFC PATCH 2/2] RDMA/efa: Add support for dmabuf memory regions
On 07/10/2021 14:40, Jason Gunthorpe wrote: > On Thu, Oct 07, 2021 at 01:43:00PM +0300, Gal Pressman wrote: > >> @@ -1491,26 +1493,29 @@ static int efa_create_pbl(struct efa_dev *dev, >> return 0; >> } >> >> -struct ib_mr *efa_reg_mr(struct ib_pd *ibpd, u64 start, u64 length, >> - u64 virt_addr, int access_flags, >> - struct ib_udata *udata) >> +static void efa_dmabuf_invalidate_cb(struct dma_buf_attachment *attach) >> +{ >> +WARN_ON_ONCE(1, >> + "Invalidate callback should not be called when memory is >> pinned\n"); >> +} >> + >> +static struct dma_buf_attach_ops efa_dmabuf_attach_ops = { >> +.allow_peer2peer = true, >> +.move_notify = efa_dmabuf_invalidate_cb, >> +}; > > Shouldn't move_notify really just be left as NULL? I mean fixing > whatever is preventing that? That's what I had in the previous RFC and I think Christian didn't really like it. >> +struct ib_mr *efa_reg_user_mr_dmabuf(struct ib_pd *ibpd, u64 start, >> + u64 length, u64 virt_addr, >> + int fd, int access_flags, >> + struct ib_udata *udata) >> +{ >> +struct efa_dev *dev = to_edev(ibpd->device); >> +struct ib_umem_dmabuf *umem_dmabuf; >> +struct efa_mr *mr; >> +int err; >> + >> +mr = efa_alloc_mr(ibpd, access_flags, udata); >> +if (IS_ERR(mr)) { >> +err = PTR_ERR(mr); >> +goto err_out; >> +} >> + >> +umem_dmabuf = ib_umem_dmabuf_get(ibpd->device, start, length, fd, >> + access_flags, _dmabuf_attach_ops); >> +if (IS_ERR(umem_dmabuf)) { >> +ibdev_dbg(>ibdev, "Failed to get dmabuf[%d]\n", err); >> +err = PTR_ERR(umem_dmabuf); >> +goto err_free; >> +} >> + >> +dma_resv_lock(umem_dmabuf->attach->dmabuf->resv, NULL); >> +err = dma_buf_pin(umem_dmabuf->attach); >> +if (err) { >> +ibdev_dbg(>ibdev, "Failed to pin dmabuf memory\n"); >> +goto err_release; >> +} >> + >> +err = ib_umem_dmabuf_map_pages(umem_dmabuf); >> +if (err) { >> +ibdev_dbg(>ibdev, "Failed to map dmabuf pages\n"); >> +goto err_unpin; >> +} >> +dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); > > If it is really this simple the core code should have this logic, > 'ib_umem_dmabuf_get_pinned()' or something Should get_pinned do just get + dma_buf_pin, or should it do ib_umem_dmabuf_map_pages as well?
Re: [RFC PATCH 2/2] RDMA/efa: Add support for dmabuf memory regions
On Thu, Oct 07, 2021 at 01:43:00PM +0300, Gal Pressman wrote: > @@ -1491,26 +1493,29 @@ static int efa_create_pbl(struct efa_dev *dev, > return 0; > } > > -struct ib_mr *efa_reg_mr(struct ib_pd *ibpd, u64 start, u64 length, > - u64 virt_addr, int access_flags, > - struct ib_udata *udata) > +static void efa_dmabuf_invalidate_cb(struct dma_buf_attachment *attach) > +{ > + WARN_ON_ONCE(1, > + "Invalidate callback should not be called when memory is > pinned\n"); > +} > + > +static struct dma_buf_attach_ops efa_dmabuf_attach_ops = { > + .allow_peer2peer = true, > + .move_notify = efa_dmabuf_invalidate_cb, > +}; Shouldn't move_notify really just be left as NULL? I mean fixing whatever is preventing that? > +struct ib_mr *efa_reg_user_mr_dmabuf(struct ib_pd *ibpd, u64 start, > + u64 length, u64 virt_addr, > + int fd, int access_flags, > + struct ib_udata *udata) > +{ > + struct efa_dev *dev = to_edev(ibpd->device); > + struct ib_umem_dmabuf *umem_dmabuf; > + struct efa_mr *mr; > + int err; > + > + mr = efa_alloc_mr(ibpd, access_flags, udata); > + if (IS_ERR(mr)) { > + err = PTR_ERR(mr); > + goto err_out; > + } > + > + umem_dmabuf = ib_umem_dmabuf_get(ibpd->device, start, length, fd, > + access_flags, _dmabuf_attach_ops); > + if (IS_ERR(umem_dmabuf)) { > + ibdev_dbg(>ibdev, "Failed to get dmabuf[%d]\n", err); > + err = PTR_ERR(umem_dmabuf); > + goto err_free; > + } > + > + dma_resv_lock(umem_dmabuf->attach->dmabuf->resv, NULL); > + err = dma_buf_pin(umem_dmabuf->attach); > + if (err) { > + ibdev_dbg(>ibdev, "Failed to pin dmabuf memory\n"); > + goto err_release; > + } > + > + err = ib_umem_dmabuf_map_pages(umem_dmabuf); > + if (err) { > + ibdev_dbg(>ibdev, "Failed to map dmabuf pages\n"); > + goto err_unpin; > + } > + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); If it is really this simple the core code should have this logic, 'ib_umem_dmabuf_get_pinned()' or something Jason