ODP Fixes
Subsystems (4)
cover ODP Fixes
From: Leon Romanovsky <[email protected]>
Hi,
Please find below three patches that fix ODP flow. The title of first patch seems not -rc material, but implementation yes. The fact that we unified code paths allowed us to fix some of the corner cases at the same time.
Thanks
Artemy Kovalyov (1):
IB/mlx5: Unify ODP MR code paths to allow extra flexibility
Yishai Hadas (2):
IB/core: Fix ODP get user pages flow IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
drivers/infiniband/core/umem_odp.c | 39 ++++++++++--------- drivers/infiniband/hw/mlx5/mem.c | 25 ------------ drivers/infiniband/hw/mlx5/mlx5_ib.h | 9 ++--- drivers/infiniband/hw/mlx5/mr.c | 58 +++++++++++----------------- drivers/infiniband/hw/mlx5/odp.c | 42 +++++++++++++++++++- 5 files changed, 87 insertions(+), 86 deletions(-)
--
2.20.1
PATCH 1/3 IB/mlx5: Unify ODP MR code paths to allow extra flexibility
From: Artemy Kovalyov <[email protected]>
Building MR translation table in ODP case requires additional flexibility, namely random access to DMA addresses. Make both direct and indirect ODP MR use same code path, separated from non-ODP MR code path.
--- drivers/infiniband/hw/mlx5/mem.c | 25 ------------ drivers/infiniband/hw/mlx5/mlx5_ib.h | 9 ++--- drivers/infiniband/hw/mlx5/mr.c | 58 +++++++++++----------------- drivers/infiniband/hw/mlx5/odp.c | 42 +++++++++++++++++++- 4 files changed, 67 insertions(+), 67 deletions(-)
drivers/infiniband/hw/mlx5/mem.c +0 -25
@@ -101,18 +101,6 @@ void mlx5_ib_cont_pages(struct ib_umem *umem, u64 addr, *count = i; } -static u64 umem_dma_to_mtt (dma_addr_t umem_dma) -{ -u64 mtt_entry = umem_dma &ODP_DMA_ADDR_MASK ; - -if (umem_dma &ODP_READ_ALLOWED_BIT ) - mtt_entry |=MLX5_IB_MTT_READ ; -if (umem_dma &ODP_WRITE_ALLOWED_BIT ) - mtt_entry |=MLX5_IB_MTT_WRITE ; - -return mtt_entry; -} -/* * Populate the given array with bus addresses from the umem. *
@@ -139,19 +127,6 @@ void __mlx5_ib_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem,struct scatterlist *sg;int entry; -if (umem->is_odp) { -WARN_ON (shift !=0 ); -WARN_ON (access_flags != (MLX5_IB_MTT_READ |MLX5_IB_MTT_WRITE )); - -for (i =0 ; i < num_pages; ++i) { -dma_addr_t pa = -to_ib_umem_odp (umem)->dma_list[offset + i]; - - pas[i] =cpu_to_be64 (umem_dma_to_mtt (pa)); - } -return ; - } - i =0 ;for_each_sg (umem->sg_head.sgl, sg, umem->nmap, entry) { len =sg_dma_len (sg) >>PAGE_SHIFT ;
drivers/infiniband/hw/mlx5/mlx5_ib.h +4 -5
@@ -1276,8 +1276,8 @@ void mlx5_ib_odp_cleanup_one(struct mlx5_ib_dev *ibdev);int __init mlx5_ib_odp_init (void );void mlx5_ib_odp_cleanup (void );void mlx5_odp_init_mr_cache_entry (struct mlx5_cache_ent *ent); -void mlx5_odp_populate_klm (struct mlx5_klm *pklm,size_t offset, -size_t nentries,struct mlx5_ib_mr *mr,int flags); +void mlx5_odp_populate_xlt (void *xlt,size_t idx,size_t nentries, +struct mlx5_ib_mr *mr,int flags); int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice,
@@ -1293,9 +1293,8 @@ static inline void mlx5_ib_odp_cleanup_one(struct mlx5_ib_dev *ibdev) {}static inline int mlx5_ib_odp_init (void ) {return 0 ; }static inline void mlx5_ib_odp_cleanup (void ) {}static inline void mlx5_odp_init_mr_cache_entry (struct mlx5_cache_ent *ent) {} -static inline void mlx5_odp_populate_klm (struct mlx5_klm *pklm,size_t offset, -size_t nentries,struct mlx5_ib_mr *mr, -int flags) {} +static inline void mlx5_odp_populate_xlt (void *xlt,size_t idx,size_t nentries, +struct mlx5_ib_mr *mr,int flags) {} static inline int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd,
drivers/infiniband/hw/mlx5/mr.c +23 -35
85 lines not on this page · raw message
drivers/infiniband/hw/mlx5/odp.c +40 -2
54 lines not on this page · raw message
Signature · 1 line
2.20.1
Re: [PATCH rdma-rc 1/3] IB/mlx5: Unify ODP MR code paths to allow extra flexibility
Quoted 8 lines · Leon Romanovsky
From: Artemy Kovalyov <[email protected]>
Building MR translation table in ODP case requires additional flexibility, namely random access to DMA addresses. Make both direct and indirect ODP MR use same code path, separated from non-ODP MR code path.
Fixes d2183c6f1958 ("RDMA/umem: Move page_shift from ib_umem to
ib_odp_umem")
What does it fix?
Jason
RE: [PATCH rdma-rc 1/3] IB/mlx5: Unify ODP MR code paths to allow extra flexibility
-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Thursday, December 19, 2019 9:07 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; Leon Romanovsky <[email protected]>; RDMA mailing list <[email protected]>; Artemy Kovalyov <[email protected]>; Aviad Yehezkel <[email protected]>; Yishai Hadas <[email protected]>
Subject: Re: [PATCH rdma-rc 1/3] IB/mlx5: Unify ODP MR code paths to allow extra flexibility
Quoted 8 lines · Leon Romanovsky
From: Artemy Kovalyov <[email protected]>
Building MR translation table in ODP case requires additional flexibility, namely random access to DMA addresses. Make both direct and indirect ODP MR use same code path, separated from non-ODP MR code path.
Fixes d2183c6f1958 ("RDMA/umem: Move page_shift from ib_umem to
ib_odp_umem")
What does it fix?
AK: fix huge page ODP MR handling, was broken in __mlx5_ib_populate_pas shift calculation
Jason
PATCH 2/3 IB/core: Fix ODP get user pages flow
In reply to Leon RomanovskyFrom: Yishai Hadas <[email protected]>
When calling get_user_pages_remote() need to work with granularity of PAGE_SIZE.
Fix the calculation of how many entries can be read in one call to consider that.
--- drivers/infiniband/core/umem_odp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/umem_odp.c +1 -1
@@ -440,7 +440,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,while (bcnt >0 ) {const size_t gup_num_pages =min_t (size_t, - (bcnt +BIT (page_shift) -1 ) >> page_shift, +ALIGN (bcnt,PAGE_SIZE ) /PAGE_SIZE ,PAGE_SIZE /sizeof (struct page *));down_read (&owning_mm->mmap_sem);
Signature · 1 line
2.20.1
Re: [PATCH rdma-rc 2/3] IB/core: Fix ODP get user pages flow
Quoted 8 lines · Leon Romanovsky
From: Yishai Hadas <[email protected]>
When calling
get_user_pages_remote()need to work with granularity ofPAGE_SIZE.Fix the calculation of how many entries can be read in one call to consider that.
Quoted 7 lines
Fixes 403cd12e2cf7 ("IB/umem: Add contiguous ODP support")Signed-off-by Yishai Hadas <[email protected]>Reviewed-by Artemy Kovalyov <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/umem_odp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Jason
PATCH 3/3 IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
In reply to Leon RomanovskyFrom: Yishai Hadas <[email protected]>
As VMAs for a given range might not be available as part of the registration phase in ODP, IB_ACCESS_HUGETLB/page_shift must be checked as part of the page fault flow.
If the application didn't mmap the backed memory with huge pages or released part of that hugepage area, an error will be set as part of the page fault flow once be detected.
--- drivers/infiniband/core/umem_odp.c | 37 +++++++++++++++--------------- 1 file changed, 19 insertions(+), 18 deletions(-)
drivers/infiniband/core/umem_odp.c +19 -18
@@ -241,22 +241,10 @@ struct ib_umem_odp *ib_umem_odp_get(struct ib_udata *udata, unsigned long addr, umem_odp->umem.owning_mm = mm = current->mm; umem_odp->notifier.ops = ops; - umem_odp->page_shift =PAGE_SHIFT ; -if (access &IB_ACCESS_HUGETLB ) { -struct vm_area_struct *vma; -struct hstate *h; - -down_read (&mm->mmap_sem); - vma =find_vma (mm,ib_umem_start (umem_odp)); -if (!vma || !is_vm_hugetlb_page (vma)) { -up_read (&mm->mmap_sem); - ret = -EINVAL ; - goto err_free; - } - h =hstate_vma (vma); - umem_odp->page_shift =huge_page_shift (h); -up_read (&mm->mmap_sem); - } +if (access &IB_ACCESS_HUGETLB ) + umem_odp->page_shift =HPAGE_SHIFT ; +else + umem_odp->page_shift =PAGE_SHIFT ; umem_odp->tgid =get_task_pid (current->group_leader,PIDTYPE_PID ); ret =ib_init_umem_odp (umem_odp, ops);
@@ -266,7 +254,6 @@ struct ib_umem_odp *ib_umem_odp_get(struct ib_udata *udata, unsigned long addr, err_put_pid:put_pid (umem_odp->tgid); -err_free:kfree (umem_odp);return ERR_PTR (ret); }
@@ -403,6 +390,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,int j, k, ret =0 , start_idx, npages =0 ;unsigned int flags =0 , page_shift;phys_addr_t p =0 ; +struct vm_area_struct **vmas;if (access_mask ==0 )return -EINVAL ;
@@ -415,6 +403,12 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,if (!local_page_list)return -ENOMEM ; + vmas = (struct vm_area_struct **)__get_free_page (GFP_KERNEL ); +if (!vmas) { + ret = -ENOMEM ; + goto out_free_page_list; + } + page_shift = umem_odp->page_shift; page_mask = ~(BIT (page_shift) -1 ); off = user_virt & (~page_mask);
@@ -453,7 +447,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,*/ npages =get_user_pages_remote (owning_process, owning_mm, user_virt, gup_num_pages, - flags, local_page_list,NULL ,NULL ); + flags, local_page_list, vmas,NULL );up_read (&owning_mm->mmap_sem);if (npages <0 ) {
@@ -477,6 +471,11 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,continue ; } +if ((1 << page_shift) >vma_kernel_pagesize (vmas[j])) { + ret = -EFAULT ; +break ; + } + ret =ib_umem_odp_map_dma_single_page ( umem_odp, k, local_page_list[j], access_mask, current_seq);
@@ -517,6 +516,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, out_put_task:if (owning_process)put_task_struct (owning_process); +free_page ((unsigned long )vmas); +out_free_page_list:free_page ((unsigned long )local_page_list);return ret; }
Signature · 1 line
2.20.1
Re: [PATCH rdma-rc 3/3] IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
Quoted 9 lines · Leon Romanovsky
drivers/infiniband/core/umem_odp.c +1
-0@@ -403,6 +390,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,int j, k, ret =0 , start_idx, npages =0 ;unsigned int flags =0 , page_shift;phys_addr_t p =0 ; +struct vm_area_struct **vmas;if (access_mask ==0 )return -EINVAL ;
drivers/infiniband/core/umem_odp.c int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, · line 410
Quoted 8 lines
drivers/infiniband/core/umem_odp.c +5
-0@@ -415,6 +403,12 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,if (!local_page_list)return -ENOMEM ; + vmas = (struct vm_area_struct **)__get_free_page (GFP_KERNEL ); +if (!vmas) { + ret = -ENOMEM ; + goto out_free_page_list; + }
I'd rather not do this on the fast path
drivers/infiniband/core/umem_odp.c int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, · line 477
Quoted 4 lines
drivers/infiniband/core/umem_odp.c +4
-0+if ((1 << page_shift) >vma_kernel_pagesize (vmas[j])) { + ret = -EFAULT ; +break ; + }
And vma's cannot be de-refenced outside the mmap_sem
There is already logic checking for linear contiguous pages:
if (user_virt & ~page_mask) {
p += PAGE_SIZE;
if (page_to_phys(local_page_list[j]) != p) {
ret = -EFAULT;
break;
}Why do we need to add the vma check?
Jason
RE: [PATCH rdma-rc 3/3] IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Thursday, December 19, 2019 8:33 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; Leon Romanovsky <[email protected]>; RDMA mailing list <[email protected]>; Artemy Kovalyov <[email protected]>; Aviad Yehezkel <[email protected]>; Yishai Hadas <[email protected]>
Subject: Re: [PATCH rdma-rc 3/3] IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
Quoted 9 lines · Leon Romanovsky
drivers/infiniband/core/umem_odp.c +1
-0@@ -403,6 +390,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,int j, k, ret =0 , start_idx, npages =0 ;unsigned int flags =0 , page_shift;phys_addr_t p =0 ; +struct vm_area_struct **vmas;if (access_mask ==0 )return -EINVAL ;
drivers/infiniband/core/umem_odp.c int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, · line 410
Quoted 8 lines
drivers/infiniband/core/umem_odp.c +5
-0@@ -415,6 +403,12 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,if (!local_page_list)return -ENOMEM ; + vmas = (struct vm_area_struct **)__get_free_page (GFP_KERNEL ); +if (!vmas) { + ret = -ENOMEM ; + goto out_free_page_list; + }
I'd rather not do this on the fast path
drivers/infiniband/core/umem_odp.c int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, · line 477
Quoted 4 lines
drivers/infiniband/core/umem_odp.c +4
-0+if ((1 << page_shift) >vma_kernel_pagesize (vmas[j])) { + ret = -EFAULT ; +break ; + }
And vma's cannot be de-refenced outside the mmap_sem
There is already logic checking for linear contiguous pages:
if (user_virt & ~page_mask) {
p += PAGE_SIZE;
if (page_to_phys(local_page_list[j]) != p) {
ret = -EFAULT;
break;
}Why do we need to add the vma check?
AK: checking for linear contiguity may be not enough, page may be transparent huge page, so in addition we also ensure its indeed persistent.
Jason
Re: [PATCH rdma-rc 3/3] IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
Quoted 3 lines · Artemy Kovalyov
AK: checking for linear contiguity may be not enough, page may be transparent huge page, so in addition we also ensure its indeed persistent.
??
I don't understand how it makes a difference. If the physical is linear we don't care if it is THP or just a lucky list of 4k pages?
Jason
Re: [PATCH rdma-rc 3/3] IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
Quoted 11 lines · Jason Gunthorpe
Quoted 3 lines · Artemy Kovalyov
AK: checking for linear contiguity may be not enough, page may be transparent huge page, so in addition we also ensure its indeed persistent.
??
I don't understand how it makes a difference. If the physical is linear we don't care if it is THP or just a lucky list of 4k pages?
Agree, once we are fine with enabling THP or lucky list of 4k there is no need for the VMA extra checking to force persistent huge pages.
In V1 will cleaned-up the VMA part from the patch and adapt the commit message accordingly.
Yishai
Re: [PATCH rdma-rc 3/3] IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
In reply to Leon RomanovskyHi Leon,
Quoted 10 lines · Leon Romanovsky
From: Yishai Hadas <[email protected]>
As VMAs for a given range might not be available as part of the registration phase in ODP, IB_ACCESS_HUGETLB/page_shift must be checked as part of the page fault flow.
If the application didn't mmap the backed memory with huge pages or released part of that hugepage area, an error will be set as part of the page fault flow once be detected.
Quoted 5 lines
Fixes 0008b84ea9af ("IB/umem: Add support to huge ODP")Signed-off-by Yishai Hadas <[email protected]>Reviewed-by Artemy Kovalyov <[email protected]>Reviewed-by Aviad Yehezkel <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>
Thanks for your patch!
Quoted 19 lines
drivers/infiniband/core/umem_odp.c +0
-13@@ -241,22 +241,10 @@ struct ib_umem_odp *ib_umem_odp_get(struct ib_udata *udata, unsigned long addr, umem_odp->umem.owning_mm = mm = current->mm; umem_odp->notifier.ops = ops; - umem_odp->page_shift =PAGE_SHIFT ; -if (access &IB_ACCESS_HUGETLB ) { -struct vm_area_struct *vma; -struct hstate *h; - -down_read (&mm->mmap_sem); - vma =find_vma (mm,ib_umem_start (umem_odp)); -if (!vma || !is_vm_hugetlb_page (vma)) { -up_read (&mm->mmap_sem); - ret = -EINVAL ; - goto err_free; - } - h =hstate_vma (vma);
drivers/infiniband/core/umem_odp.c struct ib_umem_odp *ib_umem_odp_get(struct ib_udata *udata, unsigned long addr, · line 250
Quoted 10 lines
drivers/infiniband/core/umem_odp.c +4
-3- umem_odp->page_shift =huge_page_shift (h); -up_read (&mm->mmap_sem); - } +if (access &IB_ACCESS_HUGETLB ) + umem_odp->page_shift =HPAGE_SHIFT ; +else + umem_odp->page_shift =PAGE_SHIFT ; umem_odp->tgid =get_task_pid (current->group_leader,PIDTYPE_PID ); ret =ib_init_umem_odp (umem_odp, ops);
[email protected] reports for linux-next/m68k-allmodconfig/m68k:
drivers/infiniband/core/umem_odp.c:245:26: error: 'HPAGE_SHIFT' undeclared (first use in this function); did you mean 'PAGE_SHIFT'?
Should this depend on some HUGETLBFS option?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH rdma-rc 3/3] IB/core: Fix ODP with IB_ACCESS_HUGETLB handling
Quoted 54 lines · Geert Uytterhoeven
Hi Leon,
Quoted 10 lines · Leon Romanovsky
From: Yishai Hadas <[email protected]>
As VMAs for a given range might not be available as part of the registration phase in ODP, IB_ACCESS_HUGETLB/page_shift must be checked as part of the page fault flow.
If the application didn't mmap the backed memory with huge pages or released part of that hugepage area, an error will be set as part of the page fault flow once be detected.
Quoted 5 lines
Fixes 0008b84ea9af ("IB/umem: Add support to huge ODP")Signed-off-by Yishai Hadas <[email protected]>Reviewed-by Artemy Kovalyov <[email protected]>Reviewed-by Aviad Yehezkel <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>Thanks for your patch!
Quoted 29 lines
drivers/infiniband/core/umem_odp.c +4
-16@@ -241,22 +241,10 @@ struct ib_umem_odp *ib_umem_odp_get(struct ib_udata *udata, unsigned long addr, umem_odp->umem.owning_mm = mm = current->mm; umem_odp->notifier.ops = ops; - umem_odp->page_shift = PAGE_SHIFT; - if (access & IB_ACCESS_HUGETLB) { - struct vm_area_struct *vma; - struct hstate *h; - - down_read(&mm->mmap_sem); - vma = find_vma(mm, ib_umem_start(umem_odp)); - if (!vma || !is_vm_hugetlb_page(vma)) { - up_read(&mm->mmap_sem); - ret = -EINVAL; - goto err_free; - } - h = hstate_vma(vma); - umem_odp->page_shift = huge_page_shift(h); - up_read(&mm->mmap_sem); - } + if (access & IB_ACCESS_HUGETLB) + umem_odp->page_shift = HPAGE_SHIFT; + else + umem_odp->page_shift = PAGE_SHIFT; umem_odp->tgid = get_task_pid(current->group_leader, PIDTYPE_PID);Quoted 1 line
ret =
ib_init_umem_odp(umem_odp, ops);[email protected] reports for linux-next/m68k-allmodconfig/m68k:
Quoted 5 lines
drivers/infiniband/core/umem_odp.c:245:26: error: 'HPAGE_SHIFT'
undeclared (first use in this function); did you mean 'PAGE_SHIFT'?Should this depend on some HUGETLBFS option?
Thanks for pointing on,
We would expect to use #ifdef CONFIG_HUGETLB_PAGE as done in below kernel code [1] that also used HPAGE_SHIFT.
I'll send some patch to 'for-next' to handle it.
[1] https://elixir.bootlin.com/linux/v5.3-rc7/source/drivers/misc/sgi-gru/grufault.c#L183