John Hubbard <jhubbard@xxxxxxxxxx> writes: Thanks John, have fixed most of this but a couple of comments below. > On 8/4/22 23:29, Alistair Popple wrote: [...] >> + /* >> + * We can't migrate pages with unexpected references, so drop >> + * the reference obtained by get_user_pages(). > > The get_user_pages() reference is confusing, since we only handle FOLL_PIN > here. It's hard to connect the comment to the code. Maybe a more precise > pointer to where the reference was taken would help. Ok. Is changing the reference to __get_user_pages_locked() better? (I get that this is the FOLL_PIN case, but that is the function that actually does that in __gup_longterm_locked()). >> + * folio_isolate_lru() takes a reference so the page won't be >> + * freed. > > Also confusing because it's difficult to connect the comment back to the code. > Maybe mention where folio_isolate_lru() is called from in this case? Will update this to: /* * 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. */ I'd rather not explicitly call out the function where folio_isolate_lru() is called as that could change. However it should be pretty easy to find where pages are added to movable_page_list and pages should never be on that list without being isolated anyway so this better documents what the code assumes IMHO. >> + */ >> unpin_user_page(pages[i]); >> + pages[i] = NULL; > > Is this correct? The loop covers all of nr_pages, so we are setting every > pages[i] = NULL, for non-DEV_COHERENT cases. This seems wrong. Yes, this is correct. Regardless of error or success we have to unpin the pages, and if we've unpinned the pages we need to clear them from the pages[] array. Will update the function description to include this. This is also partly why I wasn't convinced the 'goto err' error handling was better - we need to take the same actions regardless of error or not and that perhaps makes it less clear. [...] >> @@ -2049,10 +2066,10 @@ static long __gup_longterm_locked(struct mm_struct >> *mm, >> WARN_ON_ONCE(!(gup_flags & FOLL_PIN)); >> rc = check_and_migrate_movable_pages(rc, pages); >> - } while (!rc); >> + } while (rc == -EAGAIN); >> memalloc_pin_restore(flags); >> - return rc; >> + return rc ? rc : nr_pages; > > This seems to add some holes in the handling of error cases. If -EBUSY > or -ENOMEM happens, then we return an -errno, rather than the number > of pinned pages. But some pages may have been pinned. No they won't - as mentioned above migrate_unpinnable_pages() will (or at least should) always clear and unpin all pages. - Alistair > Previously it just looped if there was any error, so that couldn't > happen. > > > thanks,
