Re: [PATCH 05/19] mm/gup: introduce pin_user_pages*() and FOLL_PIN

[John Hubbard <jhubbard@nvidia.com>]

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