When pinning pages with FOLL_LONGTERM check_and_migrate_movable_pages()
is called to migrate pages out of zones which should not contain any
longterm pinned pages.
When migration succeeds all pages will have been unpinned so pinning
needs to be retried. Migration can also fail, in which case the pages
will also have been unpinned but the operation should not be retried. If
all pages are in the correct zone nothing will be unpinned and no retry
is required.
The logic in check_and_migrate_movable_pages() tracks unnecessary state
and the return codes for each case are difficult to follow. Refactor the
code to clean this up. No behaviour change is intended.
Signed-off-by: Alistair Popple <[email protected]>
Cc: "Sierra Guiza, Alejandro (Alex)" <[email protected]>
Cc: Chaitanya Kulkarni <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Felix Kuehling <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Logan Gunthorpe <[email protected]>
Cc: Miaohe Lin <[email protected]>
Cc: Muchun Song <[email protected]>
Cc: Pasha Tatashin <[email protected]>
Cc: Ralph Campbell <[email protected]>
---
Originally posted as "mm/gup.c: Simplify and fix
check_and_migrate_movable_pages() return codes"[1].
Changes from that version:
- Restore the original isolation failure behaviour and don't fail the
pup. Instead retry indefinitely.
- Unpin all pages on retry or failure rather than just failure.
Jason - I dropped your Reviewed-by. I had to remove the changes to make
error handling follow convention as we need to always unpin the pages.
We also need the list_empty() checks because we may or may not have
pages in the list if we found coherent pages. So there isn't much I
could see to simplify, but let me know if you spot some.
[1] https://lore.kernel.org/linux-mm/814dee5d3aadd38c3370eaaf438ba7eee9bf9d2b.1659399696.git-series.apopple@nvidia.com/
---
mm/gup.c | 61 +++++++++++++++++++++++++-------------------------------
1 file changed, 27 insertions(+), 34 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index c6d060dee9e0..5ce0106a92f8 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1901,20 +1901,20 @@ struct page *get_dump_page(unsigned long addr)
#ifdef CONFIG_MIGRATION
/*
- * Check whether all pages are pinnable, if so return number of pages. If some
- * pages are not pinnable, migrate them, and unpin all pages. Return zero if
- * pages were migrated, or if some pages were not successfully isolated.
- * Return negative error if migration fails.
+ * Check whether all pages are pinnable. If some pages are not pinnable migrate
+ * them and unpin all the pages. Returns -EAGAIN if pages were unpinned or zero
+ * if all pages are pinnable and in the right zone. Other errors indicate
+ * migration failure.
*/
static long check_and_migrate_movable_pages(unsigned long nr_pages,
struct page **pages,
unsigned int gup_flags)
{
- unsigned long isolation_error_count = 0, i;
+ unsigned long i;
struct folio *prev_folio = NULL;
LIST_HEAD(movable_page_list);
bool drain_allow = true, coherent_pages = false;
- int ret = 0;
+ int ret = -EAGAIN;
for (i = 0; i < nr_pages; i++) {
struct folio *folio = page_folio(pages[i]);
@@ -1946,10 +1946,10 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
unpin_user_page(&folio->page);
}
- ret = migrate_device_coherent_page(&folio->page);
- if (ret)
- goto unpin_pages;
-
+ if (migrate_device_coherent_page(&folio->page)) {
+ ret = -EBUSY;
+ break;
+ }
continue;
}
@@ -1959,9 +1959,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
* Try to move out any movable page before pinning the range.
*/
if (folio_test_hugetlb(folio)) {
- if (isolate_hugetlb(&folio->page,
- &movable_page_list))
- isolation_error_count++;
+ isolate_hugetlb(&folio->page, &movable_page_list);
continue;
}
@@ -1970,29 +1968,25 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
drain_allow = false;
}
- if (folio_isolate_lru(folio)) {
- isolation_error_count++;
+ if (folio_isolate_lru(folio))
continue;
- }
+
list_add_tail(&folio->lru, &movable_page_list);
node_stat_mod_folio(folio,
NR_ISOLATED_ANON + folio_is_file_lru(folio),
folio_nr_pages(folio));
}
- if (!list_empty(&movable_page_list) || isolation_error_count ||
- coherent_pages)
- goto unpin_pages;
-
/*
- * If list is empty, and no isolation errors, means that all pages are
- * in the correct zone.
+ * All pages are still pinned and in the correct zone.
*/
- return nr_pages;
+ if (list_empty(&movable_page_list) && !coherent_pages)
+ return 0;
-unpin_pages:
/*
- * pages[i] might be NULL if any device coherent pages were found.
+ * Unpin all pages. If device coherent pages were found
+ * migrate_deivce_coherent_page() will have already dropped the pin and
+ * set pages[i] == NULL.
*/
for (i = 0; i < nr_pages; i++) {
if (!pages[i])
@@ -2010,15 +2004,14 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
.gfp_mask = GFP_USER | __GFP_NOWARN,
};
- ret = migrate_pages(&movable_page_list, alloc_migration_target,
- NULL, (unsigned long)&mtc, MIGRATE_SYNC,
- MR_LONGTERM_PIN, NULL);
- if (ret > 0) /* number of pages not migrated */
+ if (migrate_pages(&movable_page_list, alloc_migration_target,
+ NULL, (unsigned long)&mtc, MIGRATE_SYNC,
+ MR_LONGTERM_PIN, NULL))
ret = -ENOMEM;
- }
- if (ret && !list_empty(&movable_page_list))
putback_movable_pages(&movable_page_list);
+ }
+
return ret;
}
#else
@@ -2026,7 +2019,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
struct page **pages,
unsigned int gup_flags)
{
- return nr_pages;
+ return 0;
}
#endif /* CONFIG_MIGRATION */
@@ -2054,10 +2047,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
if (rc <= 0)
break;
rc = check_and_migrate_movable_pages(rc, pages, gup_flags);
- } while (!rc);
+ } while (rc == -EAGAIN);
memalloc_pin_restore(flags);
- return rc;
+ return rc ? rc : nr_pages;
}
static bool is_valid_gup_flags(unsigned int gup_flags)
--
2.35.1
On Thu, Aug 04, 2022 at 01:22:41PM +1000, Alistair Popple wrote:
> When pinning pages with FOLL_LONGTERM check_and_migrate_movable_pages()
> is called to migrate pages out of zones which should not contain any
> longterm pinned pages.
>
> When migration succeeds all pages will have been unpinned so pinning
> needs to be retried. Migration can also fail, in which case the pages
> will also have been unpinned but the operation should not be retried. If
> all pages are in the correct zone nothing will be unpinned and no retry
> is required.
>
> The logic in check_and_migrate_movable_pages() tracks unnecessary state
> and the return codes for each case are difficult to follow. Refactor the
> code to clean this up. No behaviour change is intended.
>
> Signed-off-by: Alistair Popple <[email protected]>
> Cc: "Sierra Guiza, Alejandro (Alex)" <[email protected]>
> Cc: Chaitanya Kulkarni <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Felix Kuehling <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: John Hubbard <[email protected]>
> Cc: Logan Gunthorpe <[email protected]>
> Cc: Miaohe Lin <[email protected]>
> Cc: Muchun Song <[email protected]>
> Cc: Pasha Tatashin <[email protected]>
> Cc: Ralph Campbell <[email protected]>
>
> ---
>
> Originally posted as "mm/gup.c: Simplify and fix
> check_and_migrate_movable_pages() return codes"[1].
>
> Changes from that version:
>
> - Restore the original isolation failure behaviour and don't fail the
> pup. Instead retry indefinitely.
> - Unpin all pages on retry or failure rather than just failure.
>
> Jason - I dropped your Reviewed-by. I had to remove the changes to make
> error handling follow convention as we need to always unpin the pages.
> We also need the list_empty() checks because we may or may not have
> pages in the list if we found coherent pages. So there isn't much I
> could see to simplify, but let me know if you spot some.
I don't quite understand this, if the point is to loop on the LRU
indefinately, why not just code that? Why do we need to go around the
big loop?
Jason
On 8/3/22 20:22, Alistair Popple wrote:
> @@ -1970,29 +1968,25 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
> drain_allow = false;
> }
>
> - if (folio_isolate_lru(folio)) {
> - isolation_error_count++;
> + if (folio_isolate_lru(folio))
> continue;
> - }
> +
> list_add_tail(&folio->lru, &movable_page_list);
> node_stat_mod_folio(folio,
> NR_ISOLATED_ANON + folio_is_file_lru(folio),
> folio_nr_pages(folio));
> }
>
> - if (!list_empty(&movable_page_list) || isolation_error_count ||
> - coherent_pages)
> - goto unpin_pages;
> -
> /*
> - * If list is empty, and no isolation errors, means that all pages are
> - * in the correct zone.
> + * All pages are still pinned and in the correct zone.
> */
> - return nr_pages;
> + if (list_empty(&movable_page_list) && !coherent_pages)
> + return 0;
>
At this point, a second part of the (rather long) routine begins. If I
were refactoring this, I'd split it into a top level routine that calls
the two parts:
check_and_migrate_movable_pages()
collect_migration_candidates()
unpin_and_migrate_pages()
...which I think would further help in working with this thing.
(I realize that this is additional work. And that it's best done as a
separate patch. And it's just a suggestion.)
> -unpin_pages:
> /*
> - * pages[i] might be NULL if any device coherent pages were found.
> + * Unpin all pages. If device coherent pages were found
> + * migrate_deivce_coherent_page() will have already dropped the pin and
"migrate_device_coherent_page()"
thanks,
--
John Hubbard
NVIDIA
Jason Gunthorpe <[email protected]> writes:
> On Thu, Aug 04, 2022 at 01:22:41PM +1000, Alistair Popple wrote:
>> When pinning pages with FOLL_LONGTERM check_and_migrate_movable_pages()
>> is called to migrate pages out of zones which should not contain any
>> longterm pinned pages.
>>
>> When migration succeeds all pages will have been unpinned so pinning
>> needs to be retried. Migration can also fail, in which case the pages
>> will also have been unpinned but the operation should not be retried. If
>> all pages are in the correct zone nothing will be unpinned and no retry
>> is required.
>>
>> The logic in check_and_migrate_movable_pages() tracks unnecessary state
>> and the return codes for each case are difficult to follow. Refactor the
>> code to clean this up. No behaviour change is intended.
>>
>> Signed-off-by: Alistair Popple <[email protected]>
>> Cc: "Sierra Guiza, Alejandro (Alex)" <[email protected]>
>> Cc: Chaitanya Kulkarni <[email protected]>
>> Cc: Dan Williams <[email protected]>
>> Cc: Felix Kuehling <[email protected]>
>> Cc: Jason Gunthorpe <[email protected]>
>> Cc: John Hubbard <[email protected]>
>> Cc: Logan Gunthorpe <[email protected]>
>> Cc: Miaohe Lin <[email protected]>
>> Cc: Muchun Song <[email protected]>
>> Cc: Pasha Tatashin <[email protected]>
>> Cc: Ralph Campbell <[email protected]>
>>
>> ---
>>
>> Originally posted as "mm/gup.c: Simplify and fix
>> check_and_migrate_movable_pages() return codes"[1].
>>
>> Changes from that version:
>>
>> - Restore the original isolation failure behaviour and don't fail the
>> pup. Instead retry indefinitely.
>> - Unpin all pages on retry or failure rather than just failure.
>>
>> Jason - I dropped your Reviewed-by. I had to remove the changes to make
>> error handling follow convention as we need to always unpin the pages.
>> We also need the list_empty() checks because we may or may not have
>> pages in the list if we found coherent pages. So there isn't much I
>> could see to simplify, but let me know if you spot some.
>
> I don't quite understand this, if the point is to loop on the LRU
> indefinately, why not just code that? Why do we need to go around the
> big loop?
I assume by "big loop" you mean calling
check_and_migrate_movable_pages() again? We have to do that after the
migration anyway. Looping on the LRU indefinitely inside
check_and_migrate_movable_pages() doesn't remove that requirement so I'm
not convinced it would simplify things much.
It could also lead to deadlock because we may already have other pages
isolated from the LRU.
- Alistair
> Jason