[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 3 Mar 2019 14:37:33 -0800
From: John Hubbard <jhubbard@...dia.com>
To: Artemy Kovalyov <artemyko@...lanox.com>,
Ira Weiny <ira.weiny@...el.com>,
"john.hubbard@...il.com" <john.hubbard@...il.com>
CC: "linux-mm@...ck.org" <linux-mm@...ck.org>,
Andrew Morton <akpm@...ux-foundation.org>,
LKML <linux-kernel@...r.kernel.org>,
Jason Gunthorpe <jgg@...pe.ca>,
Doug Ledford <dledford@...hat.com>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: Re: [PATCH v2] RDMA/umem: minor bug fix and cleanup in error handling
paths
On 3/3/19 1:52 AM, Artemy Kovalyov wrote:
>
>
> On 02/03/2019 21:44, Ira Weiny wrote:
>>
>> On Sat, Mar 02, 2019 at 12:24:35PM -0800, john.hubbard@...il.com wrote:
>>> From: John Hubbard <jhubbard@...dia.com>
>>>
>>> ...
>>> 3. Dead code removal: the check for (user_virt & ~page_mask)
>>> is checking for a condition that can never happen,
>>> because earlier:
>>>
>>> user_virt = user_virt & page_mask;
>>>
>>> ...so, remove that entire phrase.
>>>
>>> bcnt -= min_t(size_t, npages << PAGE_SHIFT, bcnt);
>>> mutex_lock(&umem_odp->umem_mutex);
>>> for (j = 0; j < npages; j++, user_virt += PAGE_SIZE) {
>>> - if (user_virt & ~page_mask) {
>>> - p += PAGE_SIZE;
>>> - if (page_to_phys(local_page_list[j]) != p) {
>>> - ret = -EFAULT;
>>> - break;
>>> - }
>>> - put_page(local_page_list[j]);
>>> - continue;
>>> - }
>>> -
>>
>> I think this is trying to account for compound pages. (ie page_mask could
>> represent more than PAGE_SIZE which is what user_virt is being incrimented by.)
>> But putting the page in that case seems to be the wrong thing to do?
>>
>> Yes this was added by Artemy[1] now cc'ed.
>
> Right, this is for huge pages, please keep it.
> put_page() needed to decrement refcount of the head page.
>
OK, thanks for explaining! Artemy, while you're here, any thoughts about the
release_pages, and the change of the starting point, from the other part of the
patch:
@@ -684,9 +677,11 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp,
u64 user_virt,
mutex_unlock(&umem_odp->umem_mutex);
if (ret < 0) {
- /* Release left over pages when handling errors. */
- for (++j; j < npages; ++j)
- put_page(local_page_list[j]);
+ /*
+ * Release pages, starting at the the first page
+ * that experienced an error.
+ */
+ release_pages(&local_page_list[j], npages - j);
break;
}
}
?
thanks,
--
John Hubbard
NVIDIA
Powered by blists - more mailing lists