On 10/31/19 4:15 PM, Ira Weiny wrote:

On Wed, Oct 30, 2019 at 03:49:16PM -0700, John Hubbard wrote:

...

quoted

+ * FOLL_PIN indicates that a special kind of tracking (not just page->_refcount,
+ * but an additional pin counting system) will be invoked. This is intended for
+ * anything that gets a page reference and then touches page data (for example,
+ * Direct IO). This lets the filesystem know that some non-file-system entity is
+ * potentially changing the pages' data. In contrast to FOLL_GET (whose pages
+ * are released via put_page()), FOLL_PIN pages must be released, ultimately, by
+ * a call to put_user_page().
+ *
+ * FOLL_PIN is similar to FOLL_GET: both of these pin pages. They use different
+ * and separate refcounting mechanisms, however, and that means that each has
+ * its own acquire and release mechanisms:
+ *
+ *     FOLL_GET: get_user_pages*() to acquire, and put_page() to release.
+ *
+ *     FOLL_PIN: pin_user_pages*() or pin_longterm_pages*() to acquire, and
+ *               put_user_pages to release.
+ *
+ * FOLL_PIN and FOLL_GET are mutually exclusive.

You mean the flags are mutually exclusive for any single call, correct?
Because my first thought was that you meant that a page which was pin'ed can't
be "got".  Which I don't think is true or necessary...

Yes, you are correct. And yes you can absolutely mix get_user_pages() and 
pin_user_pages() calls on the same page(s).

OK, I'll change the wording to "mutually exclusive for a given function call".

quoted

+ *
+ * Please see Documentation/vm/pin_user_pages.rst for more information.

NIT: I think we should include this file as part of this patch...

heh. I kept hopping back and forth on this, because I've seen other patchsets that
often put Documentation/ into its own patch. But you're right, of course: it's
not right to refer to items that are not here until a later patch. I'll merge
patch 19 into this one, then.

...

quoted

@@ -1603,11 +1630,25 @@ static __always_inline long __gup_longterm_locked(struct task_struct *tsk,
  * and mm being operated on are the current task's and don't allow
  * passing of a locked parameter.  We also obviously don't pass
  * FOLL_REMOTE in here.
+ *
+ * A note on gup_flags: FOLL_PIN should only be set internally by the
+ * pin_user_page*() and pin_longterm_*() APIs, never directly by the caller.
+ * That's in order to help avoid mismatches when releasing pages:
+ * get_user_pages*() pages must be released via put_page(), while
+ * pin_user_pages*() pages must be released via put_user_page().

Rather than put this here should we put it next to the definition of FOLL_PIN?
Because now we have this text 2x...  :-/

OK, I'll move it up next to FOLL_PIN, and get rid of the 2x places in gup.c


...

quoted

+long pin_longterm_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)
+{
+	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
+	if (WARN_ON_ONCE(gup_flags & FOLL_GET))
+		return -EINVAL;
+
+	/*
+	 * FIXME: as noted in the get_user_pages_remote() implementation, it
+	 * is not yet possible to safely set FOLL_LONGTERM here. FOLL_LONGTERM
+	 * needs to be set, but for now the best we can do is a "TODO" item.
+	 */

Wait?  Why can't we set FOLL_LONGTERM here?  pin_* are new calls which are not
used yet right?

Nope, not quite! See patch #14 ("vfio, mm: pin_longterm_pages (FOLL_PIN) and 
put_user_page() conversion"), in which I'm converting an existing 
get_user_pages_remote() caller.

You set it in the other new pin_* functions?

Yes I did. Because those work already in their gup() counterparts.

thanks,

John Hubbard
NVIDIA