[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.