Re: [RFC PATCH 2/2] mm/gup: introduce vaddr_pin_pages_remote()

On 8/12/19 3:03 PM, Ira Weiny wrote:

On Sun, Aug 11, 2019 at 06:50:44PM -0700, john.hubbard@gmail.com wrote:

quoted

From: John Hubbard <jhubbard@nvidia.com>

...

quoted

+/**
+ * vaddr_pin_pages pin pages by virtual address and return the pages to the

vaddr_pin_pages_remote

Fixed in my tree.

thanks. :)

quoted

+ * user.
+ *
+ * @tsk:	the task_struct to use for page fault accounting, or
+ *		NULL if faults are not to be recorded.
+ * @mm:		mm_struct of target mm
+ * @addr:	start address
+ * @nr_pages:	number of pages to pin
+ * @gup_flags:	flags to use for the pin
+ * @pages:	array of pages returned
+ * @vaddr_pin:	initialized meta information this pin is to be associated
+ * with.
+ *
+ * This is the "vaddr_pin_pages" corresponding variant to
+ * get_user_pages_remote(), but with FOLL_PIN semantics: the implementation sets
+ * FOLL_PIN. That, in turn, means that the pages must ultimately be released
+ * by put_user_page().
+ */
+long vaddr_pin_pages_remote(struct task_struct *tsk, struct mm_struct *mm,
+			    unsigned long start, unsigned long nr_pages,
+			    unsigned int gup_flags, struct page **pages,
+			    struct vm_area_struct **vmas, int *locked,
+			    struct vaddr_pin *vaddr_pin)
+{
+	gup_flags |= FOLL_TOUCH | FOLL_REMOTE | FOLL_PIN;
+
+	return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas,
+				       locked, gup_flags, vaddr_pin);
+}
+EXPORT_SYMBOL(vaddr_pin_pages_remote);
+
 /**
  * vaddr_unpin_pages_dirty_lock - counterpart to vaddr_pin_pages
  *

@@ -2536,3 +2568,21 @@ void vaddr_unpin_pages_dirty_lock(struct page **pages, unsigned long nr_pages,
 	__put_user_pages_dirty_lock(vaddr_pin, pages, nr_pages, make_dirty);
 }
 EXPORT_SYMBOL(vaddr_unpin_pages_dirty_lock);
+
+/**
+ * vaddr_unpin_pages - simple, non-dirtying counterpart to vaddr_pin_pages
+ *
+ * @pages: array of pages returned
+ * @nr_pages: number of pages in pages
+ * @vaddr_pin: same information passed to vaddr_pin_pages
+ *
+ * Like vaddr_unpin_pages_dirty_lock, but for non-dirty pages. Useful in putting
+ * back pages in an error case: they were never made dirty.
+ */
+void vaddr_unpin_pages(struct page **pages, unsigned long nr_pages,
+		       struct vaddr_pin *vaddr_pin)
+{
+	__put_user_pages_dirty_lock(vaddr_pin, pages, nr_pages, false);
+}
+EXPORT_SYMBOL(vaddr_unpin_pages);

Rather than have another wrapping call why don't we just do this?  Would it be
so bad to just have to specify false for make_dirty?

Sure, passing in false for make_dirty is fine, and in fact, there may even be
error cases I've forgotten about that *want* to dirty the page. 

I thought about these variants, and realized that we don't generally need to 
say "lock" anymore, because we're going to forcibly use set_page_dirty_lock 
(rather than set_page_dirty) in this part of the code. And a shorter name 
is nice. Since you've dropped both "_dirty" and "_lock" from the function 
name, it's still nice and short even though we pass in make_dirty as an arg.

So that's a long-winded, "the API below looks good to me". :)

thanks,
-- 
John Hubbard
NVIDIA