[PATCH v12 2/8] mm/gup: Introduce check_and_migrate_movable_folios()
David Hildenbrand
david at redhat.com
Tue Apr 2 14:26:28 UTC 2024
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 at redhat.com>
> Cc: Matthew Wilcox <willy at infradead.org>
> Cc: Christoph Hellwig <hch at infradead.org>
> Cc: Jason Gunthorpe <jgg at nvidia.com>
> Cc: Peter Xu <peterx at redhat.com>
> Suggested-by: David Hildenbrand <david at redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy at 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
More information about the dri-devel
mailing list