Re: [PATCH v2] mm: gup: fix infinite loop within __get_longterm_locked

[David Hildenbrand <david@redhat.com>]

On 21.01.25 03:01, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> We can run into an infinite loop in __get_longterm_locked() when
> collect_longterm_unpinnable_folios() finds only folios that are isolated
> from the LRU or were never added to the LRU. This can happen when all
> folios to be pinned are never added to the LRU, for example when
> vm_ops->fault allocated pages using cma_alloc() and never added them to
> the LRU.
> 
> We incorrectly update the "collected" variable even if nothing was
> collected. Fix it by incrementing "collected" only when we isolated a
> folio and added it to the list of folios to migrate.

Last sentence now has to be "Fix it by simply taking a look at the list 
in the single caller, to see if anything was added.".

> 
> Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()")
> 
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
> v2: judge if there are unpinnable pages via list_empty and update commit
> message
> ---
> ---
>   mm/gup.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 3b75e631f369..37951fba929a 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2323,11 +2323,11 @@ static void pofs_unpin(struct pages_or_folios *pofs)
>   /*
>    * Returns the number of collected folios. Return value is always >= 0.
>    */
> -static unsigned long collect_longterm_unpinnable_folios(
> +static void collect_longterm_unpinnable_folios(
>   		struct list_head *movable_folio_list,
>   		struct pages_or_folios *pofs)
>   {
> -	unsigned long i, collected = 0;
> +	unsigned long i;

Nit: "reverse christmas tree" parameter format wants this to come after 
the "drain_allow" variable now.

>   	struct folio *prev_folio = NULL;
>   	bool drain_allow = true;
>   
> @@ -2341,8 +2341,6 @@ static unsigned long collect_longterm_unpinnable_folios(
>   		if (folio_is_longterm_pinnable(folio))
>   			continue;
>   
> -		collected++;
> -
>   		if (folio_is_device_coherent(folio))
>   			continue;
>   
> @@ -2364,8 +2362,6 @@ static unsigned long collect_longterm_unpinnable_folios(
>   				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
>   				    folio_nr_pages(folio));
>   	}
> -
> -	return collected;
>   }
>   
>   /*
> @@ -2442,11 +2438,9 @@ static long
>   check_and_migrate_movable_pages_or_folios(struct pages_or_folios *pofs)
>   {
>   	LIST_HEAD(movable_folio_list);
> -	unsigned long collected;
>   
> -	collected = collect_longterm_unpinnable_folios(&movable_folio_list,
> -						       pofs);
> -	if (!collected)
> +	collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
> +	if (list_empty(&movable_folio_list))
>   		return 0;
>   
>   	return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);

Nice, exactly what I just recommended as reply to #1 after waking up :)

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb