On 2/27/19 10:28 AM, 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
>>> --- a/drivers/infiniband/core/umem_odp.c
>>> +++ 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
> threw me a bit. Hence my question to Jason regarding the requirements of the
> driver when calling unmap. It seems like if the driver has not cleared the
> invalidate callback then there could be a potential for an unmatched
> GUP depending on what the driver is doing with the umem. But I'm
> not 100% sure I've found a problem.
>
> Ira
>
Jason, we could use your advice for this one, if you have a moment to take
a quick peek?
thanks,
--
John Hubbard
NVIDIA
