Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <634031be-4403-8758-9f24-8410a6a1a43d@fujitsu.com>
Date:   Sun, 30 Oct 2022 11:33:34 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [for-next PATCH v5 05/11] RDMA/rxe: Allow registering persistent
 flag for pmem MR only
Content-Language: en-US
To:     Jason Gunthorpe <jgg@nvidia.com>
CC:     Bob Pearson <rpearsonhpe@gmail.com>,
        Leon Romanovsky <leon@kernel.org>,
        <linux-rdma@vger.kernel.org>, Zhu Yanjun <zyjzyj2000@gmail.com>,
        <yangx.jy@fujitsu.com>, <y-goto@fujitsu.com>, <mbloch@nvidia.com>,
        <liangwenpeng@huawei.com>, <tom@talpey.com>,
        <tomasz.gromadzki@intel.com>, <dan.j.williams@intel.com>,
        <linux-kernel@vger.kernel.org>
References: <20220927055337.22630-1-lizhijian@fujitsu.com>
 <20220927055337.22630-6-lizhijian@fujitsu.com> <Y1wXD/cnIlGUANBY@nvidia.com>
From:   Li Zhijian <lizhijian@fujitsu.com>
In-Reply-To: <Y1wXD/cnIlGUANBY@nvidia.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 29/10/2022 01:53, Jason Gunthorpe wrote:
> On Tue, Sep 27, 2022 at 01:53:31PM +0800, Li Zhijian wrote:
>> @@ -122,6 +129,7 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
>>   	int			num_buf;
>>   	void			*vaddr;
>>   	int err;
>> +	bool is_pmem = false;
>>   	int i;
>>   
>>   	umem = ib_umem_get(&rxe->ib_dev, start, length, access);
>> @@ -149,6 +157,7 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
>>   	num_buf			= 0;
>>   	map = mr->map;
>>   	if (length > 0) {
>> +		is_pmem = true;
>>   		buf = map[0]->buf;
>>   
>>   		for_each_sgtable_page (&umem->sgt_append.sgt, &sg_iter, 0) {
>> @@ -166,6 +175,10 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
>>   				goto err_cleanup_map;
>>   			}
>>   
>> +			/* True only if the *whole* MR is pmem */
>> +			if (is_pmem)
>> +				is_pmem = vaddr_in_pmem(vaddr);
>> +
> I'm not so keen on this use of resources, but this should be written more
> like
>
> phys = page_to_phys(sg_page_iter_page(&sg_iter))
> region_intersects(phys + sg_iter->offset, sg_iter->length,.. )
>
> And you understand this will make memory registration of every RXE
> user a bit slower?
Good catch, i missed it before.
I tested it qemu guest in which pmem is backing to a normal file in host.
In this case, this testing take ~+9% overhead(1.2S -> 1.3S) for 1G size mr. most the time was taken by gup.

the real pmem environment will be tested later.

To minimize side effect, i updated the code to do pmem mr checking on if the require_pmem is true.

> region_intersects(phys + sg_iter->offset, sg_iter->length,.. )
I haven't fully apply this suggestion since i think my assumption that a page can only associate to a unique/same
memory zone is true. So i only check 1 byte of each page.


index 5d014cef916e..e4e7c180fa0d 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -112,6 +112,13 @@ void rxe_mr_init_dma(int access, struct rxe_mr *mr)
         mr->ibmr.type = IB_MR_TYPE_DMA;
  }

+static bool paddr_in_pmem(unsigned long paddr)
+{
+       return REGION_INTERSECTS ==
+              region_intersects(paddr, 1, IORESOURCE_MEM,
+                                IORES_DESC_PERSISTENT_MEMORY);
+}
+
  int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
                      int access, struct rxe_mr *mr)
  {
@@ -122,6 +129,7 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
         int                     num_buf;
         void                    *vaddr;
         int err;
+       bool require_pmem = access & IB_ACCESS_FLUSH_PERSISTENT;

         umem = ib_umem_get(&rxe->ib_dev, start, length, access);
         if (IS_ERR(umem)) {
@@ -149,6 +157,7 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
         num_buf                 = 0;
         map = mr->map;
         if (length > 0) {
+               struct page *pg;
                 buf = map[0]->buf;

                 for_each_sgtable_page (&umem->sgt_append.sgt, &sg_iter, 0) {
@@ -158,13 +167,20 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
                                 num_buf = 0;
                         }

-                       vaddr = page_address(sg_page_iter_page(&sg_iter));
+                       pg = sg_page_iter_page(&sg_iter);
+                       vaddr = page_address(pg);
                         if (!vaddr) {
                                 pr_warn("%s: Unable to get virtual address\n",
                                                 __func__);
                                 err = -ENOMEM;
                                 goto err_release_umem;
                         }
+
+                       if (require_pmem && !paddr_in_pmem(page_to_phys(pg))) {
+                               err = -EINVAL;
+                               goto err_release_umem;
+                       }
+
                         buf->addr = (uintptr_t)vaddr;
                         num_buf++;
                         buf++;


> And actual pmem will be painfully slow.
>
> It seems like we are doing something wrong here..
>

Do you think we don't need this patch ?


>> @@ -174,6 +187,12 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
>>   		}
>>   	}
>>   
>> +	if (!is_pmem && access & IB_ACCESS_FLUSH_PERSISTENT) {
>> +		pr_warn("Cannot register IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n");
>> +		err = -EINVAL;
>> +		goto err_release_umem;
>> +	}
> Do not pr_warn on syscall paths

Got it, will remove it.

Thanks
Zhijian


>
> Jason