Memory region could support at most 2 flush access flags:
IB_ACCESS_FLUSH_PERSISTENT and IB_ACCESS_FLUSH_GLOBAL
But we only allow user to register persistent flush flags to the pmem MR
where it has the ability of persisting data across power cycles.
So register a persistent access flag to a non-pmem MR will be rejected.
CC: Dan Williams <[email protected]>
Signed-off-by: Li Zhijian <[email protected]>
---
V5: make sure the whole MR is pmem
V4: set is_pmem more simple
V2: new scheme check is_pmem # Dan
update commit message, get rid of confusing ib_check_flush_access_flags() # Tom
---
drivers/infiniband/sw/rxe/rxe_mr.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 74a38d06332f..1da3ad5eba64 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->type = IB_MR_TYPE_DMA;
}
+static bool vaddr_in_pmem(char *vaddr)
+{
+ return REGION_INTERSECTS ==
+ region_intersects(virt_to_phys(vaddr), 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 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);
+
buf->addr = (uintptr_t)vaddr;
buf->size = PAGE_SIZE;
num_buf++;
@@ -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;
+ }
+
mr->umem = umem;
mr->access = access;
mr->offset = ib_umem_offset(umem);
--
2.31.1
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? And actual pmem will be painfully slow.
It seems like we are doing something wrong here..
> @@ -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
Jason
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