On Fri, Jan 21, 2022 at 08:06:54AM -0800, 'Ira Weiny' wrote:
> On Fri, Jan 21, 2022 at 08:58:37AM -0400, Jason Gunthorpe wrote:
> > On Thu, Jan 20, 2022 at 08:07:36PM +0800, Li, Zhijian wrote:
> >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c
> > > b/drivers/infiniband/sw/rxe/rxe_mr.c
> > > index 0621d387ccba..978fdd23665c 100644
> > > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> > > @@ -260,7 +260,8 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64
> > > length, u64 iova,
> > > num_buf = 0;
> > > }
> > >
> > > - vaddr = page_address(sg_page_iter_page(&sg_iter));
> > > + // FIXME: don't forget to kunmap_local(vaddr)
> > > + vaddr = kmap_local_page(sg_page_iter_page(&sg_iter));
> >
> > No, you can't leave the kmap open for a long time. The kmap has to
> > just be around the usage.
>
> Indeed Jason is correct here. A couple of details here. First
> kmap_local_page() is only valid within the current thread of execution. So
> what you propose above will not work at all. Second, kmap() is to be avoided.
>
> Finally, that page_address() should be avoided IMO and will be broken, at least
> for persistent memory pages, once some of my work lands.[*] Jason would know
> better, but I think page_address should be avoided in all driver code. But
> there is no clear documentation on that.
>
> Taking a quick look at rxe_mr.c buf->addr is only used in rxe_mr_init_user().
^^^^^^^^^^^^^^
Sorry... I meant rxe_mr_copy()...
> You need to kmap_local_page() around that access. What else is struct
> rxe_phys_buf->addr used for? Can you just map the page when you need it in
> rxe_mr_init_user()?
rxe_mr_copy()...
Ira
>
> If you must create a mapping that is permanent you could look at vmap().
>
> Ira
>
> >
> > Jason
