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

From: "lizhijian@fujitsu.com" <lizhijian@fujitsu.com>
To: Ira Weiny <ira.weiny@intel.com>, Jason Gunthorpe <jgg@nvidia.com>
Cc: "lizhijian@fujitsu.com" <lizhijian@fujitsu.com>,
	"yangx.jy@fujitsu.com" <yangx.jy@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>
Subject: Re: [RFC PATCH v2 2/2] RDMA/rxe: Support RDMA Atomic Write operation
Date: Mon, 24 Jan 2022 03:47:59 +0000	[thread overview]
Message-ID: <33225e9c-a1f4-4b20-0a57-ac3a42e7fb52@fujitsu.com> (raw)
In-Reply-To: <20220121160841.GD773547@iweiny-DESK2.sc.intel.com>

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