On Sat, Mar 02, 2019 at 09:25:42PM -0400, Jason Gunthorpe wrote:
> On Wed, Feb 27, 2019 at 10:28:13AM -0800, Ira Weiny wrote:
> > On Tue, Feb 26, 2019 at 05:22:44PM -0800, John Hubbard wrote:
> > > On 2/26/19 5:10 PM, Ira Weiny wrote:
> > > > John,
> > > >
> > > > With your put_user_page patchset, Is there a reason you don't call
> > > > put_user_page() in the 2 spots shown in the diff below?
> > > >
> > > > And Jason, in the second case I find it odd that the driver needs to clear the
> > > > invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages()
> > > > call such that put_user_page() is called. I am correct that this is how the
> > > > pages finally get put in the end?
> > > >
> > > > Ira
> > > >
> > > > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> > > > index 730b2fa0942e..a63f5eda02ca 100644
> > > > +++ b/drivers/infiniband/core/umem_odp.c
> > > > @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
> > > > if (ret < 0) {
> > > > /* Release left over pages when handling errors. */
> > > > for (++j; j < npages; ++j)
> > > > - put_page(local_page_list[j]);
> > > > + put_user_page(local_page_list[j]);
> > >
> > > Hi Ira,
> > >
> > > This one I somehow just overlooked. It definitely should have been a put_user_page()
> > > call.
> > >
> > > > break;
> > > > }
> > > > }
> > > > @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt,
> > > > }
> > > > /* on demand pinning support */
> > > > if (!umem->context->invalidate_range)
> > > > - put_page(page);
> > > > + put_user_page(page);
> > >
> > >
> > > ...and this one, I didn't recognize as being part of the pool of pages that
> > > were acquired via get_user_pages(). But assuming that it is acquired via
> > > get_user_pages(), then of course it also should be changed to put_user_page(),
> > > you're right.
> >
> > Yes I'm 99% sure this matches up with the get_user_pages_remote() in
> > ib_umem_odp_map_dma_pages(). But the check for the
> > invalidate_range() callback
>
> invalidate_range does not change, it always set to something if the
> driver supports ODP, and since this function accepts a ib_umem_odp it
> cannot be called on non-ODP umem's..
>
> In this case I assume the code can never be called??
Ok, that is what I thought... So where is the matching put_[user_]page
(currently put_page but John's change would be user page) for the
get_user_pages_remote() call? (CC Haggai)
I think this is it and I think it is a bug to have the check here. Or perhaps
the check should be:
if (umem->context->invalidate_range)
...
???
Ira
>
> Jason
