On Sun, Mar 03, 2019 at 06:00:35AM +0000, Haggai Eran wrote:
> On 3/2/2019 9:54 PM, Ira Weiny wrote:
> > 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 also think this code is never called. IIRC, in order to split the
> on-demand paging patch-set we had an intermediate mode called "on-demand
> pinning", where the driver would only support page faults, but not
> invalidations. In this mode the page references are taken when a page
> fault occurs, but are held until the MR is released. As far as I know no
> driver uses this mode, so it is probably safe to remove it.
>
> >
> > 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)
> > ...
> > ???
>
> The thought behind this interface was that invalidate_range callback
> would either be set or cleared during the per mm or the umem creation,
> so it is stable between the map and unmap calls. If it was set during
> map, then the page reference is immediately released there, so there's
> no need for another put_page in unmap.
I'm missing something. I don't see any check for the invalidate_range callback
in ib_umem_odp_map_dma_pages(). So I'm still missing when the pages which get
pinned by the GUP_remote() call there are "put".
It seems like the ib_umem_odp_[un]map_dma_pages() should mirror each other.
Ira
>
> Regards,
> Haggai
