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

[John Hubbard]

On 2020-06-25 22:26, Souptick Joarder wrote:
> On Thu, Jun 25, 2020 at 11:19 AM John Hubbard <jhubbard@xxxxxxxxxx> 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