Re: [RFC PATCH v2 2/2] RDMA/rxe: Support RDMA Atomic Write operation

From: "yangx.jy@fujitsu.com" <yangx.jy@fujitsu.com>
To: Ira Weiny <ira.weiny@intel.com>, Jason Gunthorpe <jgg@nvidia.com>
Cc: "lizhijian@fujitsu.com" <lizhijian@fujitsu.com>,
	"lizhijian@fujitsu.com" <lizhijian@fujitsu.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"tom@talpey.com" <tom@talpey.com>,
	"yanjun.zhu@linux.dev" <yanjun.zhu@linux.dev>,
	"rpearsonhpe@gmail.com" <rpearsonhpe@gmail.com>,
	"y-goto@fujitsu.com" <y-goto@fujitsu.com>,
	"tomasz.gromadzki@intel.com" <tomasz.gromadzki@intel.com>,
	"hch@lst.de" <hch@lst.de>
Subject: Re: [RFC PATCH v2 2/2] RDMA/rxe: Support RDMA Atomic Write operation
Date: Thu, 27 Jan 2022 09:37:59 +0000	[thread overview]
Message-ID: <b3b322be-a718-5fb8-11e2-05ee783f1086@fujitsu.com> (raw)
In-Reply-To: <20220121160654.GC773547@iweiny-DESK2.sc.intel.com>

On 2022/1/22 0:06, 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.

Cc Christoph Hellwig

Hi Jason, Ira

Sorry for the late reply.

Do you mean we have to consider that some allocated pages come from high 
memory?

I think INFINIBAND_VIRT_DMA kconfig[1] has ensured that all allocated 
pages have a kernel virtual address.

In this case, is it OK to call page_address() directly?

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b1e678bf290db5a76f1b6a9f7c381310e03440d6

> 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.

Could you explain why page_address() will be broken for persistent 
memory pages?

Sorry, I am not familiar with the principle of persistent memory.

Best Regards,

Xiao Yang

>
> Taking a quick look at rxe_mr.c buf->addr is only used in rxe_mr_init_user().
> 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()?
>
> If you must create a mapping that is permanent you could look at vmap().
>
> Ira
>
>> Jason