lkml.org 
[lkml]   [2020]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC PATCH] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()
On Fri, Jun 19, 2020 at 1:00 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2020-06-18 20:12, Souptick Joarder wrote:
> > On Wed, Jun 17, 2020 at 11:29 PM Boris Ostrovsky
> > <boris.ostrovsky@oracle.com> 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.
>

Ok.

>
> >>>
> >>> [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@gmail.com>
> >>> Cc: John Hubbard <jhubbard@nvidia.com>
> >>> ---
> >>> 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.

I read the code again. I think, this check is needed to handle a scenario when
lock_pages() return -ENOSPC. Better to keep this check. Let me post v2 of this
RFC for a clear view.

>
>
> > 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

\
 
 \ /
  Last update: 2020-06-22 20:45    [W:0.131 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site