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

[David Hildenbrand <david@redhat.com>]

On 25.02.24 08:56, 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. Since they
> are also used by check_and_migrate_movable_pages(), a temporary
> array is used to collect and share the folios with these functions.
> 
> 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>
> ---
>   mm/gup.c | 129 +++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 92 insertions(+), 37 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 0a45eda6aaeb..1410af954a4e 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2099,20 +2099,24 @@ struct page *get_dump_page(unsigned long addr)
>   
>   #ifdef CONFIG_MIGRATION
>   /*
> - * Returns the number of collected pages. Return value is always >= 0.
> + * Returns the number of collected folios. Return value is always >= 0.
>    */
> -static unsigned long collect_longterm_unpinnable_pages(
> -					struct list_head *movable_page_list,
> -					unsigned long nr_pages,
> +static unsigned long collect_longterm_unpinnable_folios(
> +					struct list_head *movable_folio_list,
> +					unsigned long nr_folios,
> +					struct folio **folios,
>   					struct page **pages)

This function really shouldn't consume both folios and pages.

Either use "folios" and handle the conversion from pages->folios in the 
caller, or handle it similar to release_pages() where we can pass either 
and simply always do page_folio() on the given pointer, using 
essentially an abstracted pointer type and always calling page_folio() 
on that thing.

The easiest is likely to just do the page->folio conversion in the 
caller by looping over the arrays once more. See below.

Temporary memory allocation can be avoided by using an abstracted 
pointer type.

[...]

>   
> +		folio = folios[i];
>   		if (folio == prev_folio)
>   			continue;
>   		prev_folio = folio;
> @@ -2126,7 +2130,7 @@ static unsigned long collect_longterm_unpinnable_pages(
>   			continue;
>   
>   		if (folio_test_hugetlb(folio)) {
> -			isolate_hugetlb(folio, movable_page_list);
> +			isolate_hugetlb(folio, movable_folio_list);
>   			continue;
>   		}
>   
> @@ -2138,7 +2142,7 @@ static unsigned long collect_longterm_unpinnable_pages(
>   		if (!folio_isolate_lru(folio))
>   			continue;
>   
> -		list_add_tail(&folio->lru, movable_page_list);
> +		list_add_tail(&folio->lru, movable_folio_list);
>   		node_stat_mod_folio(folio,
>   				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
>   				    folio_nr_pages(folio));
> @@ -2148,27 +2152,28 @@ static unsigned long collect_longterm_unpinnable_pages(
>   }
>   
>   /*
> - * Unpins all pages and migrates device coherent pages and movable_page_list.
> - * Returns -EAGAIN if all pages were successfully migrated or -errno for failure
> - * (or partial success).
> + * Unpins all folios and migrates device coherent folios and movable_folio_list.
> + * Returns -EAGAIN if all folios were successfully migrated or -errno for
> + * failure (or partial success).
>    */
> -static int migrate_longterm_unpinnable_pages(
> -					struct list_head *movable_page_list,
> -					unsigned long nr_pages,
> -					struct page **pages)
> +static int migrate_longterm_unpinnable_folios(
> +					struct list_head *movable_folio_list,
> +					unsigned long nr_folios,
> +					struct folio **folios)
>   {
>   	int ret;
>   	unsigned long i;
>   
> -	for (i = 0; i < nr_pages; i++) {
> -		struct folio *folio = page_folio(pages[i]);
> +	for (i = 0; i < nr_folios; i++) {
> +		struct folio *folio = folios[i];
>   
>   		if (folio_is_device_coherent(folio)) {
>   			/*
> -			 * Migration will fail if the page is pinned, so convert
> -			 * the pin on the source page to a normal reference.
> +			 * Migration will fail if the folio is pinned, so
> +			 * convert the pin on the source folio to a normal
> +			 * reference.
>   			 */
> -			pages[i] = NULL;
> +			folios[i] = NULL;
>   			folio_get(folio);
>   			gup_put_folio(folio, 1, FOLL_PIN);
>   
> @@ -2181,23 +2186,23 @@ static int migrate_longterm_unpinnable_pages(
>   		}
>   
>   		/*
> -		 * We can't migrate pages with unexpected references, so drop
> +		 * We can't migrate folios with unexpected references, so drop
>   		 * the reference obtained by __get_user_pages_locked().
> -		 * Migrating pages have been added to movable_page_list after
> +		 * Migrating folios have been added to movable_folio_list after
>   		 * calling folio_isolate_lru() which takes a reference so the
> -		 * page won't be freed if it's migrating.
> +		 * folio won't be freed if it's migrating.
>   		 */
> -		unpin_user_page(pages[i]);
> -		pages[i] = NULL;
> +		unpin_folio(folios[i]);

Aha, that's where you call unpin_folio() on an anon folio. Then simply 
drop the sanity check from inside unpin_folio() in patch #1.

> +		folios[i] = NULL;
>   	}
>   
> -	if (!list_empty(movable_page_list)) {
> +	if (!list_empty(movable_folio_list)) {
>   		struct migration_target_control mtc = {
>   			.nid = NUMA_NO_NODE,
>   			.gfp_mask = GFP_USER | __GFP_NOWARN,
>   		};
>   
> -		if (migrate_pages(movable_page_list, alloc_migration_target,
> +		if (migrate_pages(movable_folio_list, alloc_migration_target,
>   				  NULL, (unsigned long)&mtc, MIGRATE_SYNC,
>   				  MR_LONGTERM_PIN, NULL)) {
>   			ret = -ENOMEM;
> @@ -2205,15 +2210,15 @@ static int migrate_longterm_unpinnable_pages(
>   		}
>   	}
>   
> -	putback_movable_pages(movable_page_list);
> +	putback_movable_pages(movable_folio_list);

This really needs a cleanup (independent of your work). We should rename 
it to putback_movable_folios: it only operates on folios.

>   
>   	return -EAGAIN;
>   
>   err:
> -	for (i = 0; i < nr_pages; i++)
> -		if (pages[i])
> -			unpin_user_page(pages[i]);
> -	putback_movable_pages(movable_page_list);
> +	for (i = 0; i < nr_folios; i++)
> +		if (folios[i])
> +			unpin_folio(folios[i]);

Can unpin_folios() be used?

> +	putback_movable_pages(movable_folio_list);
>   
>   	return ret;
>   }
> @@ -2237,16 +2242,60 @@ static int migrate_longterm_unpinnable_pages(
>   static long check_and_migrate_movable_pages(unsigned long nr_pages,
>   					    struct page **pages)
>   {
> +	unsigned long nr_folios = nr_pages;
>   	unsigned long collected;
> -	LIST_HEAD(movable_page_list);
> +	LIST_HEAD(movable_folio_list);
> +	struct folio **folios;
> +	long ret;
>   
> -	collected = collect_longterm_unpinnable_pages(&movable_page_list,
> -						nr_pages, pages);
> +	folios = kmalloc_array(nr_folios, sizeof(*folios), GFP_KERNEL);
> +	if (!folios)
> +		return -ENOMEM;
> +
> +	collected = collect_longterm_unpinnable_folios(&movable_folio_list,
> +						       nr_folios, folios,
> +						       pages);
> +	if (!collected) {
> +		kfree(folios);
> +		return 0;
> +	}
> +
> +	ret = migrate_longterm_unpinnable_folios(&movable_folio_list,
> +						 nr_folios, folios);
> +	kfree(folios);
> +	return ret;


This function should likely be a pure rapper around 
check_and_migrate_movable_folios(). For example:

static long check_and_migrate_movable_pages(unsigned long nr_pages,
					    struct page **pages)
{
	struct folio **folios;
	long ret;

	folios = kmalloc_array(nr_folios, sizeof(*folios), GFP_KERNEL);
	if (!folios)
		return -ENOMEM;

	/* TODO, convert all pages to folios. */

	ret = check_and_migrate_movable_folios(nr_folios, folios);

	kfree(folios);
	return ret;
}

> +}
> +
> +/*
> + * Check whether all folios are *allowed* to be pinned. Rather confusingly, all

... "to be pinned possibly forever ("longterm")".

> + * 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.
> + *


[...]

-- 
Cheers,

David / dhildenb