On Tue, Apr 30, 2019 at 05:25:29PM +0000, Saleem, Shiraz wrote:
> >@@ -409,13 +405,13 @@ int ib_umem_odp_get(struct ib_umem_odp
> >*umem_odp, int access)
> > struct hstate *h;
> >
> > down_read(&mm->mmap_sem);
> >- vma = find_vma(mm, ib_umem_start(umem));
> >+ vma = find_vma(mm, ib_umem_start(umem_odp));
> > if (!vma || !is_vm_hugetlb_page(vma)) {
> > up_read(&mm->mmap_sem);
> > return -EINVAL;
> > }
> > h = hstate_vma(vma);
> >- umem->page_shift = huge_page_shift(h);
> >+ umem_odp->page_shift = huge_page_shift(h);
> > up_read(&mm->mmap_sem);
> > umem->hugetlb = 1;
>
> Is scanning VMA to compute huge page shift a good idea? Why does ODP MRs need to do this?
> Shouldn't we be looking at addresses after DMA translation to upgrade page shift? And isn't
> this already in place with mlx5_ib_cont_pages()?
It is unclear to me if ODP is really working as best it can here or
not, it may not really support huge contiguous regions as well as
could be expected :\
> >@@ -595,15 +588,15 @@ int ib_umem_odp_map_dma_pages(struct
> >ib_umem_odp *umem_odp, u64 user_virt,
> > if (access_mask == 0)
> > return -EINVAL;
> >
> >- if (user_virt < ib_umem_start(umem) ||
> >- user_virt + bcnt > ib_umem_end(umem))
> >+ if (user_virt < ib_umem_start(umem_odp) ||
> >+ user_virt + bcnt > ib_umem_end(umem_odp))
> > return -EFAULT;
> >
> > local_page_list = (struct page **)__get_free_page(GFP_KERNEL);
> > if (!local_page_list)
> > return -ENOMEM;
> >
> >- page_shift = umem->page_shift;
> >+ page_shift = umem_odp->page_shift;
>
> page_shift should be unsigned int?
I think many of the types are wrong in the ODP code.. Certainly
shouldn't be int
> >diff --git a/drivers/infiniband/hw/mlx5/mem.c b/drivers/infiniband/hw/mlx5/mem.c
> >index 9f90be296ee0..1619fdb6526b 100644
> >+++ b/drivers/infiniband/hw/mlx5/mem.c
> >@@ -55,9 +55,10 @@ void mlx5_ib_cont_pages(struct ib_umem *umem, u64
> >addr,
> > int i = 0;
> > struct scatterlist *sg;
> > int entry;
> >- unsigned long page_shift = umem->page_shift;
> >
> > if (umem->is_odp) {
> >+ unsigned long page_shift = to_ib_umem_odp(umem)-
> >>page_shift;
>
> unsigned int?
Sure
> >+static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp
> >+*umem_odp) {
> >+ return (ib_umem_end(umem_odp) - ib_umem_start(umem_odp)) >>
> >+PAGE_SHIFT; }
> >+
> > /*
> > * The lower 2 bits of the DMA address signal the R/W permissions for
> > * the entry. To upgrade the permissions, provide the appropriate
>
> Reviewed-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
Thanks!
Jason
