Jason & Ira On 22/01/2022 00:08, Ira Weiny wrote: > 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. Thanks you all for the details explanation. It *does* make sense. >> 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()... iova_to_vaddr() is another user by process_atomic(). > >> You need to kmap_local_page() around that access. What else is struct >> rxe_phys_buf->addr used for? IIUC, rxe_phys_buf->addr is used for permanently mapping a user space address to kernel space address. So in RDMA scene, REMOTE side can access(read/write) LOCAL memory allocated by user space application directly. >> Can you just map the page when you need it in >> rxe_mr_init_user()? It can be in theory, but I'm not sure. @Jason, what's your opinion. > rxe_mr_copy()... > > Ira > >> If you must create a mapping that is permanent you could look at vmap(). Well, let me see Thanks Zhijian >> >> Ira >> >>> Jason
