| Message ID | 6b61e9bf7c65f78524db32ba3e65a7eb6b8a76a0.1660269441.git-series.apopple@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] mm/gup.c: Don't pass gup_flags to check_and_migrate_movable_pages() | expand |
On 8/11/22 19:13, 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> OK, I've finally convinced myself that this is a correct transformation. This cleanup does help clarify things, definitely. I've got two documentation additions (and changes) to suggest, below, and a couple of too-long lines, but the code itself looks good, so with those tweaks or something approximating them, please feel free to add: Reviewed-by: John Hubbard <jhubbard@nvidia.com> ... > +/* > + * 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. > + */ Instead of the above, I'd like to suggest this: /* * 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 * this routine. * * If any pages in the range are not allowed to be pinned, then this routine * will migrate those pages away, unpin all the pages 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 pages in the range are allowed to be pinned, then * this routine leaves all pages pinned and returns zero for success. */ > +static long check_and_migrate_movable_pages(unsigned long nr_pages, > + struct page **pages) > +{ > + int ret; > + unsigned long collected; > + LIST_HEAD(movable_page_list); > + > + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); There is no reason to exceed 80 cols here. > + if (!collected) > + return 0; > + > + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); Nor here. ... > @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm, > break; ...and in this routine, let's fortify the comment like so: @@ -2068,7 +2078,15 @@ static long __gup_longterm_locked(struct mm_struct *mm, if (!(gup_flags & FOLL_LONGTERM)) return __get_user_pages_locked(mm, start, nr_pages, pages, vmas, NULL, gup_flags); - /* check_and_migrate_movable_pages() assumes pages have been pinned. */ + /* + * If we get to this point then FOLL_LONGTERM is set. And FOLL_LONGTERM + * implies FOLL_PIN (although the reverse is not true). And that, in + * turn, makes it correct to unconditionally call + * check_and_migrate_movable_pages(), which assumes pages have been + * pinned via FOLL_PIN. + * + * Enforce the above reasoning, by asserting that FOLL_PIN is set: + */ if (WARN_ON(!(gup_flags & FOLL_PIN))) return -EINVAL; flags = memalloc_pin_save(); ...and with that, it's actually possible for the reader to work their way through this story, I think. thanks,
On Fri, Aug 12, 2022 at 12:13:09PM +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> > > --- This seems like the cleanest version yet! > +static long check_and_migrate_movable_pages(unsigned long nr_pages, > + struct page **pages) > +{ > + int ret; > + unsigned long collected; > + LIST_HEAD(movable_page_list); > + > + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); > + if (!collected) > + return 0; > + > + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); > + if (!ret) > + return -EAGAIN; > + else > + return ret; I would drop the else path and just return zero Arguably migrate_longterm_unpinnable_pages() should do the same? > @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm, > break; > > rc = check_and_migrate_movable_pages(rc, pages); > - } while (!rc); > + } while (rc == -EAGAIN); Since the only reader only cares about errno or not errno.. But no biggie either way Thanks, Jason
Jason Gunthorpe <jgg@nvidia.com> writes: > On Fri, Aug 12, 2022 at 12:13:09PM +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> >> >> --- > > This seems like the cleanest version yet! Thanks, the feedback from John and yourself has been very useful! >> +static long check_and_migrate_movable_pages(unsigned long nr_pages, >> + struct page **pages) >> +{ >> + int ret; >> + unsigned long collected; >> + LIST_HEAD(movable_page_list); >> + >> + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); >> + if (!collected) >> + return 0; >> + >> + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); >> + if (!ret) >> + return -EAGAIN; >> + else >> + return ret; > > I would drop the else path and just return zero Unless I've misunderstood you I don't think that's correct though. check_and_migrate_movable_pages() needs to return one of 3 conditions: - 0: All pages are in the correct zone and are still pinned (ie. "success"). - -EAGAIN: Some pages were migrated, all pages need re-pinning. - errno: Migration failed, pins were dropped and PUP should fail. John's suggested comment update spells this out more clearly. > Arguably migrate_longterm_unpinnable_pages() should do the same? migrate_longterm_unpinnable_pages() returns 0 for success, errno for some kind of "permanent" failure that needs propagating. >> @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm, >> break; >> >> rc = check_and_migrate_movable_pages(rc, pages); >> - } while (!rc); >> + } while (rc == -EAGAIN); > > Since the only reader only cares about errno or not errno.. See above. Errors other than -EAGAIN should not be retried. > But no biggie either way So will leave this as for now at least. > Thanks, > Jason
On Tue, Aug 16, 2022 at 03:29:20PM +1000, Alistair Popple wrote: > >> +static long check_and_migrate_movable_pages(unsigned long nr_pages, > >> + struct page **pages) > >> +{ > >> + int ret; > >> + unsigned long collected; > >> + LIST_HEAD(movable_page_list); > >> + > >> + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); > >> + if (!collected) > >> + return 0; > >> + > >> + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); > >> + if (!ret) > >> + return -EAGAIN; > >> + else > >> + return ret; > > > > I would drop the else path and just return zero > > Unless I've misunderstood you I don't think that's correct though. > check_and_migrate_movable_pages() needs to return one of 3 conditions: > > - 0: All pages are in the correct zone and are still pinned (ie. "success"). > - -EAGAIN: Some pages were migrated, all pages need re-pinning. > - errno: Migration failed, pins were dropped and PUP should fail. > > John's suggested comment update spells this out more clearly. But that is not what it does.. if (!ret) == if (ret != 0) return -EAGAIN thus ret == 0 return ret == return 0 Jason
Jason Gunthorpe <jgg@nvidia.com> writes: > On Tue, Aug 16, 2022 at 03:29:20PM +1000, Alistair Popple wrote: > >> >> +static long check_and_migrate_movable_pages(unsigned long nr_pages, >> >> + struct page **pages) >> >> +{ >> >> + int ret; >> >> + unsigned long collected; >> >> + LIST_HEAD(movable_page_list); >> >> + >> >> + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); >> >> + if (!collected) >> >> + return 0; >> >> + >> >> + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); >> >> + if (!ret) >> >> + return -EAGAIN; >> >> + else >> >> + return ret; >> > >> > I would drop the else path and just return zero >> >> Unless I've misunderstood you I don't think that's correct though. >> check_and_migrate_movable_pages() needs to return one of 3 conditions: >> >> - 0: All pages are in the correct zone and are still pinned (ie. "success"). >> - -EAGAIN: Some pages were migrated, all pages need re-pinning. >> - errno: Migration failed, pins were dropped and PUP should fail. >> >> John's suggested comment update spells this out more clearly. > > But that is not what it does.. > > if (!ret) == if (ret != 0) Huh? Unless I'm misinterpretting you or am really behind on coffee if (!ret) is equivalent to if (ret == 0), not if (ret != 0). Ie: if (ret == 0) return -EAGAIN; else return !0; Thus we will never return 0 from the above snippet. > return -EAGAIN > thus ret == 0 > return ret == return 0 > > Jason
On Wed, Aug 17, 2022 at 12:01:58PM +1000, Alistair Popple wrote: > > Jason Gunthorpe <jgg@nvidia.com> writes: > > > On Tue, Aug 16, 2022 at 03:29:20PM +1000, Alistair Popple wrote: > > > >> >> +static long check_and_migrate_movable_pages(unsigned long nr_pages, > >> >> + struct page **pages) > >> >> +{ > >> >> + int ret; > >> >> + unsigned long collected; > >> >> + LIST_HEAD(movable_page_list); > >> >> + > >> >> + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); > >> >> + if (!collected) > >> >> + return 0; > >> >> + > >> >> + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); > >> >> + if (!ret) > >> >> + return -EAGAIN; > >> >> + else > >> >> + return ret; > >> > > >> > I would drop the else path and just return zero > >> > >> Unless I've misunderstood you I don't think that's correct though. > >> check_and_migrate_movable_pages() needs to return one of 3 conditions: > >> > >> - 0: All pages are in the correct zone and are still pinned (ie. "success"). > >> - -EAGAIN: Some pages were migrated, all pages need re-pinning. > >> - errno: Migration failed, pins were dropped and PUP should fail. > >> > >> John's suggested comment update spells this out more clearly. > > > > But that is not what it does.. > > > > if (!ret) == if (ret != 0) > > Huh? Unless I'm misinterpretting you or am really behind on coffee > if (!ret) is equivalent to if (ret == 0), not if (ret != 0). Oh Dear, maybe I am still a bit jetlagged Regardless, it is confusingly written :) Sorry, Jason
On 8/17/22 05:09, Jason Gunthorpe wrote: >>> if (!ret) == if (ret != 0) >> >> Huh? Unless I'm misinterpretting you or am really behind on coffee >> if (!ret) is equivalent to if (ret == 0), not if (ret != 0). Although the !ret idiom is a huge, huge part of kernel coding practice, it also adds a slight bit of unnecessary mental translation (go ahead, flame me hard, I know I'm asking for it...but...see above, after all. Pixels have been spilt working through it). And also... > > Oh Dear, maybe I am still a bit jetlagged > > Regardless, it is confusingly written :) > ...this is a slightly tricky and unusual error case, so it helps to be extra clear by comparing against zero, and even adding a few short comments. How's this look to you: collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); if (collected == 0) return 0; ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); /* If we got here, we have some unpinnable pages... */ if (ret == 0) { /* * ...and we successfully migrated those pages. Which means that * the caller should retry the operation now. */ ret = -EAGAIN; } return ret; thanks,
On Wed, Aug 17, 2022 at 01:35:12PM -0700, John Hubbard wrote: > How's this look to you: > > collected = collect_longterm_unpinnable_pages(&movable_page_list, > nr_pages, pages); > if (collected == 0) > return 0; > > ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, > pages); > > /* If we got here, we have some unpinnable pages... */ > > if (ret == 0) { > /* > * ...and we successfully migrated those pages. Which means that > * the caller should retry the operation now. > */ > ret = -EAGAIN; return -EAGAIN > } > > return ret; But why return 0 from the helper function in the first place? Jason
On 8/17/22 15:50, Jason Gunthorpe wrote: > > But why return 0 from the helper function in the first place? > The caller is looking for three distinct return code cases: 0, -EAGAIN, or some other -errno. Unless you restructure more, you need all three of those cases. The 0 case leads to the caller, __gup_longterm_locked, returning nr_pages. Here is the comment block that I recommended earlier in this thread, that documents those cases: https://lore.kernel.org/r/dc8fd102-ba30-d980-bdbb-11f39326103d@nvidia.com thanks,
Jason Gunthorpe <jgg@nvidia.com> writes: > On Wed, Aug 17, 2022 at 01:35:12PM -0700, John Hubbard wrote: >> How's this look to you: I agree, I think all the refactoring left this written in a weird way. I was going to suggest this though: collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); if (collected == 0) return 0; ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); if (ret) return ret; return -EAGAIN; Which IMHO looks at lot more normal and sane than what I had. >> collected = collect_longterm_unpinnable_pages(&movable_page_list, >> nr_pages, pages); >> if (collected == 0) >> return 0; >> >> ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, >> pages); >> >> /* If we got here, we have some unpinnable pages... */ >> >> if (ret == 0) { >> /* >> * ...and we successfully migrated those pages. Which means that >> * the caller should retry the operation now. >> */ >> ret = -EAGAIN; > > return -EAGAIN > >> } >> >> return ret; > > But why return 0 from the helper function in the first place? To stick with the paradigm of 0 == success. Ie. migrate_longterm_unpinnable_pages() successfully migrated everything requested. I don't feel particularly strongly about this though - happy to return -EAGAIN directly from migrate_longterm_unpinnable_pages() and just pass that return code up the stack if others think it's clearer. > Jason
On Thu, Aug 18, 2022 at 09:24:28AM +1000, Alistair Popple wrote: > collected = collect_longterm_unpinnable_pages(&movable_page_list, > nr_pages, pages); > if (collected == 0) > return 0; > > ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, > pages); > if (ret) > return ret; > > return -EAGAIN; > > Which IMHO looks at lot more normal and sane than what I had. That isn't "success oriented flow" :) > > But why return 0 from the helper function in the first place? > > To stick with the paradigm of 0 == success. Ie. But it doesn't mean success if we squashed it to EAGAIN here. -EAGAIN == retry 0 == success, real success everything else == -ERRNO, failure Stick with one convection in all the functions in this file in this grouping. Jason
diff --git a/mm/gup.c b/mm/gup.c index a2baa8b..b181d97 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1901,19 +1901,16 @@ 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. + * Returns the number of collected pages. Return value is always >= 0. */ -static long check_and_migrate_movable_pages(unsigned long nr_pages, - struct page **pages) +static unsigned long collect_longterm_unpinnable_pages( + struct list_head *movable_page_list, + unsigned long nr_pages, + struct page **pages) { - unsigned long isolation_error_count = 0, i; + unsigned long i, collected = 0; struct folio *prev_folio = NULL; - LIST_HEAD(movable_page_list); - bool drain_allow = true, coherent_pages = false; - int ret = 0; + bool drain_allow = true; for (i = 0; i < nr_pages; i++) { struct folio *folio = page_folio(pages[i]); @@ -1922,43 +1919,16 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages, continue; prev_folio = folio; - /* - * Device coherent pages are managed by a driver and should not - * be pinned indefinitely as it prevents the driver moving the - * page. So when trying to pin with FOLL_LONGTERM instead try - * to migrate the page out of device memory. - */ - if (folio_is_device_coherent(folio)) { - /* - * We always want a new GUP lookup with device coherent - * pages. - */ - pages[i] = 0; - coherent_pages = true; - - /* - * Migration will fail if the page is pinned, so convert - * the pin on the source page to a normal reference. - */ - get_page(&folio->page); - unpin_user_page(&folio->page); + if (folio_is_longterm_pinnable(folio)) + continue; - ret = migrate_device_coherent_page(&folio->page); - if (ret) - goto unpin_pages; + collected++; + if (folio_is_device_coherent(folio)) continue; - } - if (folio_is_longterm_pinnable(folio)) - continue; - /* - * 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; } @@ -1967,59 +1937,117 @@ 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); + + 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; + return collected; +} - /* - * If list is empty, and no isolation errors, means that all pages are - * in the correct zone. - */ - return nr_pages; +/* + * Unpins all pages and migrates device coherent pages and movable_page_list. + * Returns zero if all pages 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) +{ + int ret; + unsigned long i; -unpin_pages: - /* - * pages[i] might be NULL if any device coherent pages were found. - */ for (i = 0; i < nr_pages; i++) { - if (!pages[i]) + struct folio *folio = page_folio(pages[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. + */ + pages[i] = NULL; + folio_get(folio); + gup_put_folio(folio, 1, FOLL_PIN); + + if (migrate_device_coherent_page(&folio->page)) { + ret = -EBUSY; + goto err; + } + continue; + } + /* + * We can't migrate pages with unexpected references, so drop + * the reference obtained by __get_user_pages_locked(). + * Migrating pages have been added to movable_page_list after + * calling folio_isolate_lru() which takes a reference so the + * page won't be freed if it's migrating. + */ unpin_user_page(pages[i]); + pages[i] = NULL; } - if (!list_empty(&movable_page_list)) { + if (!list_empty(movable_page_list)) { struct migration_target_control mtc = { .nid = NUMA_NO_NODE, .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; + goto err; + } } - if (ret && !list_empty(&movable_page_list)) - putback_movable_pages(&movable_page_list); + putback_movable_pages(movable_page_list); + + return 0; + +err: + for (i = 0; i < nr_pages; i++) + if (pages[i]) + unpin_user_page(pages[i]); + putback_movable_pages(movable_page_list); + return ret; } + +/* + * 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) +{ + int ret; + unsigned long collected; + LIST_HEAD(movable_page_list); + + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); + if (!collected) + return 0; + + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); + if (!ret) + return -EAGAIN; + else + return ret; +} #else static long check_and_migrate_movable_pages(unsigned long nr_pages, struct page **pages) { - return nr_pages; + return 0; } #endif /* CONFIG_MIGRATION */ @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm, break; rc = check_and_migrate_movable_pages(rc, pages); - } 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> --- Changes for v4: - Use folio directly instead of page based functions and folio->page. Changes for v3: - Improved comments (thanks John). - Fix up inconsistent int/long/unsigned long. - Rename (migrate|collect)_unpinnable_pages to (migrate|collect)_longterm_unpinnable_pages() as suggested by David. Changes for v2: - Split into different functions as suggested by John. - Made error handling more conventional as requested by Jason. 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. [1] https://lore.kernel.org/linux-mm/814dee5d3aadd38c3370eaaf438ba7eee9bf9d2b.1659399696.git-series.apopple@nvidia.com/ --- mm/gup.c | 166 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 97 insertions(+), 69 deletions(-)