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

[John Hubbard]

On 2020-06-18 20:12, Souptick Joarder wrote:
> On Wed, Jun 17, 2020 at 11:29 PM Boris Ostrovsky
> <boris.ostrovsky@xxxxxxxxxx> wrote:
> > On 6/16/20 11:14 PM, Souptick Joarder wrote:
> > > In 2019, we introduced pin_user_pages*() and now we are converting
> > > get_user_pages*() to the new API as appropriate. [1] & [2] could
> > > be referred for more information.


Ideally, the commit description should say which case, in
pin_user_pages.rst, that this is.


> > > [1] Documentation/core-api/pin_user_pages.rst
> > >
> > > [2] "Explicit pinning of user-space pages":
> > >          https://lwn.net/Articles/807108/
> > >
> > > Signed-off-by: Souptick Joarder <jrdr.linux@xxxxxxxxx>
> > > Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> > > ---
> > > Hi,
> > >
> > > I have compile tested this patch but unable to run-time test,
> > > so any testing help is much appriciated.
> > >
> > > Also have a question, why the existing code is not marking the
> > > pages dirty (since it did FOLL_WRITE) ?
> >
> >
> > Indeed, seems to me it should. Paul?

Definitely good to get an answer from an expert in this code, but
meanwhile, it's reasonable to just mark them dirty. Below...

> > >   drivers/xen/privcmd.c | 7 ++-----
> > >   1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > > index a250d11..543739e 100644
> > > --- a/drivers/xen/privcmd.c
> > > +++ b/drivers/xen/privcmd.c
> > > @@ -594,7 +594,7 @@ static int lock_pages(
> > >                if (requested > nr_pages)
> > >                        return -ENOSPC;
> > >
> > > -             pinned = get_user_pages_fast(
> > > +             pinned = pin_user_pages_fast(
> > >                        (unsigned long) kbufs[i].uptr,
> > >                        requested, FOLL_WRITE, pages);
> > >                if (pinned < 0)
> > > @@ -614,10 +614,7 @@ static void unlock_pages(struct page *pages[], unsigned int nr_pages)
> > >        if (!pages)
> > >                return;
> > >
> > > -     for (i = 0; i < nr_pages; i++) {
> > > -             if (pages[i])
> > > -                     put_page(pages[i]);
> > > -     }
> > > +     unpin_user_pages(pages, nr_pages);


...so just use unpin_user_pages_dirty_lock() here, I think.


> > Why are you no longer checking for valid pages?
>
> My understanding is, in case of lock_pages() end up returning partial
> mapped pages,
> we should pass no. of partial mapped pages to unlock_pages(), not nr_pages.
> This will avoid checking extra check to validate the pages[i].
>
> and if lock_pages() returns 0 in success, anyway we have all the pages[i] valid.
> I will try to correct it in v2.
>
> But I agree, there is no harm to check for pages[i] and I believe,


Generally, it *is* harmful to do unnecessary checks, in most code, but especially
in most kernel code. If you can convince yourself that the check for null pages
is redundant here, then please let's remove that check. The code becomes then
becomes shorter, simpler, and faster.


> unpin_user_pages()
> is the right place to do so.
>
> John any thought ?


So far I haven't seen any cases to justify changing the implementation of
unpin_user_pages().


thanks,
--
John Hubbard
NVIDIA