On 10/31/19 4:15 PM, Ira Weiny wrote:
> On Wed, Oct 30, 2019 at 03:49:16PM -0700, John Hubbard wrote:
...
>> + * 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".
>
>> + *
>> + * 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.
...
>> @@ -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
...
>> +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
