Re: [PATCH v13 2/8] mm/gup: Introduce check_and_migrate_movable_folios()

[David Hildenbrand <david@redhat.com>]

On 04.04.24 09:26, Vivek Kasireddy wrote:
> This helper is the folio equivalent of check_and_migrate_movable_pages().
> Therefore, all the rules that apply to check_and_migrate_movable_pages()
> also apply to this one as well. Currently, this helper is only used by
> memfd_pin_folios().
> 
> This patch also includes changes to rename and convert the internal
> functions collect_longterm_unpinnable_pages() and
> migrate_longterm_unpinnable_pages() to work on folios. As a result,
> check_and_migrate_movable_pages() is now a wrapper around
> check_and_migrate_movable_folios().
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Peter Xu <peterx@redhat.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---

[...]

> +/*
> + * Check whether all folios are *allowed* to be pinned indefinitely (longterm).
> + * 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.
> + *
> + * 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,
> + * then this routine leaves all folios pinned and returns zero for success.
> + */
> +static long check_and_migrate_movable_folios(unsigned long nr_folios,
> +					     struct folio **folios)
> +{
> +	unsigned long collected;
> +	LIST_HEAD(movable_folio_list);
> +
> +	collected = collect_longterm_unpinnable_folios(&movable_folio_list,
> +						       nr_folios, folios);
> +	if (!collected)
> +		return 0;
> +
> +	return migrate_longterm_unpinnable_folios(&movable_folio_list,
> +						  nr_folios, folios);
> +}
> +
>   /*
>    * Check whether all pages are *allowed* to be pinned. Rather confusingly, all
>    * pages in the range are required to be pinned via FOLL_PIN, before calling

Likely we should just drop that comment and refer to 
check_and_migrate_movable_folios() instead. No need to duplicate all that.

> @@ -2555,16 +2585,20 @@ static int migrate_longterm_unpinnable_pages(
>   static long check_and_migrate_movable_pages(unsigned long nr_pages,
>   					    struct page **pages)
>   {
> -	unsigned long collected;
> -	LIST_HEAD(movable_page_list);
> +	struct folio **folios;
> +	long i, ret;
>   
> -	collected = collect_longterm_unpinnable_pages(&movable_page_list,
> -						nr_pages, pages);
> -	if (!collected)
> -		return 0;
> +	folios = kmalloc_array(nr_pages, sizeof(*folios), GFP_KERNEL);
> +	if (!folios)
> +		return -ENOMEM;
>   
> -	return migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages,
> -						pages);
> +	for (i = 0; i < nr_pages; i++)
> +		folios[i] = page_folio(pages[i]);


I wonder if we have to handle pages[i] being NULL. Hopefully not :)

Looks straight forward now:

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

-- 
Cheers,

David / dhildenb