Re: [PATCH 1/8] mm/gup: have internal functions get the mmap_read_lock()

[John Hubbard <jhubbard@nvidia.com>]

On 1/17/23 07:58, Jason Gunthorpe wrote:
> __get_user_pages_locked() and __gup_longterm_locked() both require the
> mmap lock to be held. They have a slightly unusual locked parameter that
> is used to allow these functions to unlock and relock the mmap lock and
> convey that fact to the caller.
> 
> Several places wrapper these functions with a simple mmap_read_lock() just
> so they can follow the optimized locked protocol.
> 
> Consolidate this internally to the functions. Allow internal callers to
> set locked = 0 to cause the functions to obtain and release the lock on
> their own.
> 
> Reorganize __gup_longterm_locked() to use the autolocking in
> __get_user_pages_locked().
> 
> Replace all the places obtaining the mmap_read_lock() just to call
> __get_user_pages_locked() with the new mechanism. Replace all the internal
> callers of get_user_pages_unlocked() with direct calls to
> __gup_longterm_locked() using the new mechanism.
> 
> A following patch will add assertions ensuring the external interface
> continues to always pass in locked = 1.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   mm/gup.c | 92 +++++++++++++++++++++++++++++---------------------------
>   1 file changed, 47 insertions(+), 45 deletions(-)

Nice cleanup already in just this first patch...

> 
> diff --git a/mm/gup.c b/mm/gup.c
> index f45a3a5be53a48..3a9f764165f50b 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1343,13 +1343,22 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
>   						unsigned int flags)
>   {
>   	long ret, pages_done;
> -	bool lock_dropped;
> +	bool lock_dropped = false;

A small thing, but the naming and the associated comments later are now
inaccurate. Let's add function-level documentation to explain the new
calling convention for __get_user_pages_locked(), change the name of
lock_dropped to must_unlock, add a comment mid-function, and correct the
comment at the end of the function. Like this:

diff --git a/mm/gup.c b/mm/gup.c
index c0f86ff7848a..5b7c09be7442 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1331,8 +1331,17 @@ bool gup_signal_pending(unsigned int flags)
  }

  /*
- * Please note that this function, unlike __get_user_pages will not
- * return 0 for nr_pages > 0 without FOLL_NOWAIT
+ * Locking: (*locked == 1) means that the mmap_lock has already been acquired by
+ * the caller. This function may drop the mmap_lock. If it does so, then it will
+ * set (*locked = 0).
+ *
+ * (*locked == 0) means that the caller expects this function to acquire and
+ * drop the mmap_lock. Therefore, the value of *locked will still be zero when
+ * the function returns, even though it may have changed temporarily during
+ * function execution.
+ *
+ * Please note that this function, unlike __get_user_pages(), will not
+ * return 0 for nr_pages > 0, unless FOLL_NOWAIT is used.
   */
  long __get_user_pages_locked(struct mm_struct *mm,
  						unsigned long start,
@@ -1343,7 +1352,7 @@ long __get_user_pages_locked(struct mm_struct *mm,
  						unsigned int flags)
  {
  	long ret, pages_done;
-	bool lock_dropped = false;
+	bool must_unlock = false;

  	if (locked) {
  		/* if VM_FAULT_RETRY can be returned, vmas become invalid */
@@ -1357,7 +1366,7 @@ long __get_user_pages_locked(struct mm_struct *mm,
  	if (locked && !*locked) {
  		if (mmap_read_lock_killable(mm))
  			return -EAGAIN;
-		lock_dropped = true;
+		must_unlock = true;
  		*locked = 1;
  	}

@@ -1412,7 +1421,9 @@ long __get_user_pages_locked(struct mm_struct *mm,
  		if (likely(pages))
  			pages += ret;
  		start += ret << PAGE_SHIFT;
-		lock_dropped = true;
+
+		/* The lock was temporarily dropped, so we must unlock later: */
+		must_unlock = true;

  retry:
  		/*
@@ -1459,10 +1470,11 @@ long __get_user_pages_locked(struct mm_struct *mm,
  			pages++;
  		start += PAGE_SIZE;
  	}
-	if (lock_dropped && *locked) {
+	if (must_unlock && *locked) {
  		/*
-		 * We must let the caller know we temporarily dropped the lock
-		 * and so the critical section protected by it was lost.
+		 * We either temporarily dropped the lock, or the caller
+		 * requested that we both acquire and drop the lock. Either way,
+		 * we must now unlock, and notify the caller of that state.
  		 */
  		mmap_read_unlock(mm);
  		*locked = 0;


...

> @@ -3180,14 +3183,13 @@ EXPORT_SYMBOL(pin_user_pages);
>   long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
>   			     struct page **pages, unsigned int gup_flags)
>   {
> -	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
> -	if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> -		return -EINVAL;

Why are we dropping the assertion check for FOLL_GET?


thanks,
-- 
John Hubbard
NVIDIA