On 1/17/23 07:58, Jason Gunthorpe wrote:
> Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It
> is a more explicit replacement for locked != NULL. This clears the way for
> passing in locked = 1, without intending that the lock can be unlocked.
>
> Set the flag in all cases where it is used, eg locked is present in the
> external interface or locked is used internally with locked = 0.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
> include/linux/mm.h | 1 +
> mm/gup.c | 31 +++++++++++++++++++------------
> 2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index f3f196e4d66d6f..7496a5c8acede1 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
> #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */
> #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */
> #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */
> +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */
Looks good. Admin note: that this will conflict with commit b5054174ac7c
("mm: move FOLL_* defs to mm_types.h), which is in mm-stable. Because
the FOLL_* items were moved.
Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>
thanks,
--
John Hubbard
NVIDIA
>
> /*
> * FOLL_PIN and FOLL_LONGTERM may be used in various combinations with each
> diff --git a/mm/gup.c b/mm/gup.c
> index d203e268793b9c..4c360fb05cf3e4 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -896,7 +896,7 @@ static int faultin_page(struct vm_area_struct *vma,
> fault_flags |= FAULT_FLAG_WRITE;
> if (*flags & FOLL_REMOTE)
> fault_flags |= FAULT_FLAG_REMOTE;
> - if (locked) {
> + if (*flags & FOLL_UNLOCK) {
> fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
> /*
> * FAULT_FLAG_INTERRUPTIBLE is opt-in. GUP callers must set
> @@ -1379,9 +1379,11 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
> for (;;) {
> ret = __get_user_pages(mm, start, nr_pages, flags, pages,
> vmas, locked);
> - if (!locked)
> + if (!(flags & FOLL_UNLOCK)) {
> /* VM_FAULT_RETRY couldn't trigger, bypass */
> - return ret;
> + pages_done = ret;
> + break;
> + }
>
> /* VM_FAULT_RETRY or VM_FAULT_COMPLETED cannot return errors */
> if (!*locked) {
> @@ -2106,12 +2108,20 @@ static bool is_valid_gup_args(struct page **pages, struct vm_area_struct **vmas,
> * interfaces:
> * - FOLL_PIN/FOLL_TRIED/FOLL_FAST_ONLY is internal only
> * - FOLL_REMOTE is internal only and used on follow_page()
> + * - FOLL_UNLOCK is internal only and used if locked is !NULL
> */
> - if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_TRIED |
> + if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_TRIED | FOLL_UNLOCK |
> FOLL_REMOTE | FOLL_FAST_ONLY)))
> return false;
>
> gup_flags |= to_set;
> + if (locked) {
> + /* At the external interface locked must be set */
> + if (WARN_ON_ONCE(*locked != 1))
> + return false;
> +
> + gup_flags |= FOLL_UNLOCK;
> + }
>
> /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> @@ -2126,10 +2136,6 @@ static bool is_valid_gup_args(struct page **pages, struct vm_area_struct **vmas,
> if (WARN_ON_ONCE((gup_flags & (FOLL_GET | FOLL_PIN)) && !pages))
> return false;
>
> - /* At the external interface locked must be set */
> - if (WARN_ON_ONCE(locked && *locked != 1))
> - return false;
> -
> /* We want to allow the pgmap to be hot-unplugged at all times */
> if (WARN_ON_ONCE((gup_flags & FOLL_LONGTERM) &&
> (gup_flags & FOLL_PCI_P2PDMA)))
> @@ -2139,7 +2145,7 @@ static bool is_valid_gup_args(struct page **pages, struct vm_area_struct **vmas,
> * Can't use VMAs with locked, as locked allows GUP to unlock
> * which invalidates the vmas array
> */
> - if (WARN_ON_ONCE(vmas && locked))
> + if (WARN_ON_ONCE(vmas && (gup_flags & FOLL_UNLOCK)))
> return false;
>
> *gup_flags_p = gup_flags;
> @@ -2279,7 +2285,8 @@ long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> {
> int locked = 0;
>
> - if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, FOLL_TOUCH))
> + if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags,
> + FOLL_TOUCH | FOLL_UNLOCK))
> return -EINVAL;
>
> return __get_user_pages_locked(current->mm, start, nr_pages, pages,
> @@ -2967,7 +2974,7 @@ static int internal_get_user_pages_fast(unsigned long start,
> pages += nr_pinned;
> ret = __gup_longterm_locked(current->mm, start, nr_pages - nr_pinned,
> pages, NULL, &locked,
> - gup_flags | FOLL_TOUCH);
> + gup_flags | FOLL_TOUCH | FOLL_UNLOCK);
> if (ret < 0) {
> /*
> * The caller has to unpin the pages we already pinned so
> @@ -3194,7 +3201,7 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> int locked = 0;
>
> if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags,
> - FOLL_PIN | FOLL_TOUCH))
> + FOLL_PIN | FOLL_TOUCH | FOLL_UNLOCK))
> return 0;
>
> return __gup_longterm_locked(current->mm, start, nr_pages, pages, NULL,
