Re: [PATCH v3] mm/gup: stop leaking pinned pages in low memory conditions

[Alistair Popple <apopple@nvidia.com>]

John Hubbard <jhubbard@nvidia.com> writes:

[...]

> diff --git a/mm/gup.c b/mm/gup.c
> index a82890b46a36..4637dab7b54f 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2394,20 +2394,25 @@ static int migrate_longterm_unpinnable_folios(
>  }
>  
>  /*
> - * Check whether all folios are *allowed* to be pinned indefinitely (longterm).
> + * Check whether all folios are *allowed* to be pinned indefinitely (long term).
>   * Rather confusingly, all folios in the range are required to be pinned via
>   * FOLL_PIN, before calling this routine.
>   *
> - * If any folios in the range are not allowed to be pinned, then this routine
> - * will migrate those folios away, unpin all the folios in the range and return
> - * -EAGAIN. The caller should re-pin the entire range with FOLL_PIN and then
> - * call this routine again.
> + * Return values:
>   *
> - * If an error other than -EAGAIN occurs, this indicates a migration failure.
> - * The caller should give up, and propagate the error back up the call stack.
> - *
> - * If everything is OK and all folios in the range are allowed to be pinned,
> + * 0: if everything is OK and all folios in the range are allowed to be pinned,
>   * then this routine leaves all folios pinned and returns zero for success.
> + *
> + * -EAGAIN: if any folios in the range are not allowed to be pinned, then this
> + * routine will migrate those folios away, unpin all the folios in the range. If
> + * migration of the entire set of folios succeeds, then -EAGAIN is returned. The
> + * caller should re-pin the entire range with FOLL_PIN and then call this
> + * routine again.
> + *
> + * -ENOMEM, or any other -errno: if an error *other* than -EAGAIN occurs, this
> + * indicates a migration failure. The caller should give up, and propagate the
> + * error back up the call stack. The caller does not need to unpin any folios in
> + * that case, because this routine will do the unpinning.

Where does the unpinning happen in this case though? I can see it
happens for the specific case of failing allocation of the folio array
in check_and_migrate_movable_pages(), but what about for the other error
conditions?

>   */
>  static long check_and_migrate_movable_folios(unsigned long nr_folios,
>  					     struct folio **folios)
> @@ -2425,10 +2430,8 @@ static long check_and_migrate_movable_folios(unsigned long nr_folios,
>  }
>  
>  /*
> - * This routine just converts all the pages in the @pages array to folios and
> - * calls check_and_migrate_movable_folios() to do the heavy lifting.
> - *
> - * Please see the check_and_migrate_movable_folios() documentation for details.
> + * Return values and behavior are the same as those for
> + * check_and_migrate_movable_folios().
>   */
>  static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  					    struct page **pages)
> @@ -2437,8 +2440,10 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  	long i, ret;
>  
>  	folios = kmalloc_array(nr_pages, sizeof(*folios), GFP_KERNEL);
> -	if (!folios)
> +	if (!folios) {
> +		unpin_user_pages(pages, nr_pages);

ie. Doesn't this unpinning need to happen in
check_and_migrate_movable_folios()?

>  		return -ENOMEM;
> +	}
>  
>  	for (i = 0; i < nr_pages; i++)
>  		folios[i] = page_folio(pages[i]);
>
> base-commit: b04ae0f45168973edb658ac2385045ac13c5aca7