Hi Jason,
Thank you for your comments. My replies below.
On Thu, Dec 17, 2020 at 3:50 PM Jason Gunthorpe jgg@ziepe.ca wrote:
On Thu, Dec 17, 2020 at 01:52:41PM -0500, Pavel Tatashin wrote:
+/*
- Verify that there are no unpinnable (movable) pages, if so return true.
- Otherwise an unpinnable pages is found return false, and unpin all pages.
- */
+static bool check_and_unpin_pages(unsigned long nr_pages, struct page **pages,
unsigned int gup_flags)+{
unsigned long i, step;for (i = 0; i < nr_pages; i += step) {struct page *head = compound_head(pages[i]);step = compound_nr(head) - (pages[i] - head);You can't assume that all of a compound head is in the pages array, this assumption would only work inside the page walkers if the page was found in a PMD or something.
I am not sure I understand your comment. The compound head is not taken from the pages array, and not assumed to be in it. It is exactly the same logic as that we currently have: https://soleen.com/source/xref/linux/mm/gup.c?r=a00cda3f#1565
if (gup_flags & FOLL_PIN) {unpin_user_pages(pages, nr_pages);So we throw everything away? Why? That isn't how the old algorithm worked
It is exactly like the old algorithm worked: if there are pages to be migrated (not pinnable pages) we unpinned everything. See here: https://soleen.com/source/xref/linux/mm/gup.c?r=a00cda3f#1603
If cma_pages_list is not empty unpin everything. The list is not empty if we isolated some pages, we isolated some pages if there are some pages which are not pinnable. Now, we do exactly the same thing, but cleaner, and handle errors. We must unpin everything because if we fail, no pages should stay pinned, and also if we migrated some pages, the pages array must be updated, so we need to call __get_user_pages_locked() pin and repopulated pages array.
@@ -1654,22 +1664,55 @@ static long __gup_longterm_locked(struct mm_struct *mm, struct vm_area_struct **vmas, unsigned int gup_flags) {
unsigned long flags = 0;
int migrate_retry = 0;int isolate_retry = 0;unsigned int flags; long rc;
if (gup_flags & FOLL_LONGTERM)flags = memalloc_pin_save();
if (!(gup_flags & FOLL_LONGTERM))return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,NULL, gup_flags);
rc = __get_user_pages_locked(mm, start, nr_pages, pages, vmas, NULL,gup_flags);
/** Without FOLL_WRITE fault handler may return zero page, which can* be in a movable zone, and also will fail to isolate during migration,* thus the longterm pin will fail.*/gup_flags &= FOLL_WRITE;Is &= what you mean here? |= right?
Right, I meant |=.
Seems like we've ended up in a weird place if FOLL_LONGTERM always includes FOLL_WRITE. Putting the zero page in ZONE_MOVABLE seems like a bad idea, no?
I am not sure, I just found this problem during testing, and this is the solution I am proposing. I am worried about limiting the zero page to a non movable zone, but let's see what others think about this.
/** Migration may fail, we retry before giving up. Also, because after* migration pages[] becomes outdated, we unpin and repin all pages* in the range, so pages array is repopulated with new values.* Also, because of this we cannot retry migration failures in a loop* without pinning/unpinnig pages.*/The old algorithm made continuous forward progress and only went back to the first migration point.
That is not right, the old code went back to the beginning. Making continuous progress is possible, but we won't see any performance betnefit from it, because migration failures is already exception scenarios where machine is under memory stress. The truth is if we fail to migrate it is unlikely will succeed if we retry right away, so giving some time between retries may be even beneficial. Also with continious progress we need to take care of some corner cases where we need to unpin already succeeded pages in case if forward progress is not possible. Also, adjust pages array, start address etc.
for (; ; ) {while (true)?
Hm, the same thing? :)
rc = __get_user_pages_locked(mm, start, nr_pages, pages, vmas,NULL, gup_flags);
/* Return if error or if all pages are pinnable */if (rc <= 0 || check_and_unpin_pages(rc, pages, gup_flags))break;So we sweep the pages list twice now?
Yes, but O(N) is the same. No new operation is added. Before we had something like this:
while (npages) check if pinnable isolate while (npages) unpin migrate while (npages) pin
Now: while(npages) check if pinnable while(npages) unpin while(npages) isolate migrate pin
/* Some pages are not pinnable, migrate them */rc = migrate_movable_pages(rc, pages);/** If there is an error, and we tried maximum number of times* bail out. Notice: we return an error code, and all pages are* unpinned*/if (rc < 0 && migrate_retry++ >= PINNABLE_MIGRATE_MAX) {break;} else if (rc > 0 && isolate_retry++ >= PINNABLE_ISOLATE_MAX) {rc = -EBUSY;I don't like this at all. It shouldn't be so flakey
Can you do migration without the LRU?
I do not think it is possible, we must isolate pages before migration.
Pasha