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@xxxxxxxxxx>
---
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
