| Message ID | 20220804032241.859891-1-apopple@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup.c: Refactor check_and_migrate_movable_pages() | expand |
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 <apopple@nvidia.com> > Cc: "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com> > Cc: Chaitanya Kulkarni <kch@nvidia.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Felix Kuehling <Felix.Kuehling@amd.com> > Cc: Jason Gunthorpe <jgg@nvidia.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Logan Gunthorpe <logang@deltatee.com> > Cc: Miaohe Lin <linmiaohe@huawei.com> > Cc: Muchun Song <songmuchun@bytedance.com> > Cc: Pasha Tatashin <pasha.tatashin@soleen.com> > Cc: Ralph Campbell <rcampbell@nvidia.com> > > --- > > 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,
Jason Gunthorpe <jgg@nvidia.com> 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 <apopple@nvidia.com> >> Cc: "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com> >> Cc: Chaitanya Kulkarni <kch@nvidia.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >> Cc: Jason Gunthorpe <jgg@nvidia.com> >> Cc: John Hubbard <jhubbard@nvidia.com> >> Cc: Logan Gunthorpe <logang@deltatee.com> >> Cc: Miaohe Lin <linmiaohe@huawei.com> >> Cc: Muchun Song <songmuchun@bytedance.com> >> Cc: Pasha Tatashin <pasha.tatashin@soleen.com> >> Cc: Ralph Campbell <rcampbell@nvidia.com> >> >> --- >> >> 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
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)
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 <apopple@nvidia.com> Cc: "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com> Cc: Chaitanya Kulkarni <kch@nvidia.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Felix Kuehling <Felix.Kuehling@amd.com> Cc: Jason Gunthorpe <jgg@nvidia.com> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Logan Gunthorpe <logang@deltatee.com> Cc: Miaohe Lin <linmiaohe@huawei.com> Cc: Muchun Song <songmuchun@bytedance.com> Cc: Pasha Tatashin <pasha.tatashin@soleen.com> Cc: Ralph Campbell <rcampbell@nvidia.com> --- 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(-)