Re: [PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[John Hubbard <jhubbard@nvidia.com>]

On 2020-06-25 22:26, Souptick Joarder wrote:
> On Thu, Jun 25, 2020 at 11:19 AM John Hubbard <jhubbard@nvidia.com> wrote:
>> On 2020-06-24 20:02, Souptick Joarder wrote:
...
>>> @@ -612,13 +612,7 @@ static int lock_pages(
>>>
>>>    static void unlock_pages(struct page *pages[], unsigned int nr_pages)
>>>    {
>>> -     unsigned int i;
>>> -
>>> -     for (i = 0; i < nr_pages; i++) {
>>> -             if (!PageDirty(page))
>>> -                     set_page_dirty_lock(page);
>>> -             put_page(pages[i]);
>>> -     }
>>> +     unpin_user_pages_dirty_lock(pages, nr_pages, 1);
>>
>> "true", not "1", is the correct way to call that function.
> 
> Ok.
> 
>>
>> Also, this approach changes the behavior slightly, but I think it's

Correction, I forgot that I put that same if(!PageDirty(page)) check into
unpin_user_pages_dirty_lock(). So it doesn't change behavior. That's good.

>> reasonable to just set_page_dirty_lock() on the whole range--hard to
>> see much benefit in checking PageDirty first.
> 
> unpin_user_pages_dirty_lock() internally will do the same check after
> patch [2/2]
> So I thought to keep old and new code in sync. Shall we avoid this check ?
> 
Just leave it as you have it, but of course use "true" instead of 1, please.


thanks,
-- 
John Hubbard
NVIDIA