On Fri, Dec 18, 2020 at 9:19 AM Jason Gunthorpe jgg@ziepe.ca wrote:
On Thu, Dec 17, 2020 at 05:02:03PM -0500, Pavel Tatashin wrote:
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
Oh, that existing logic is wrong too :( Another bug.
I do not think there is a bug.
You can't skip pages in the pages[] array under the assumption they are contiguous. ie the i+=step is wrong.
If pages[i] is part of a compound page, the other parts of this page must be sequential in this array for this compound page (it might start in the middle through). If they are not sequential then the translation will be broken, as these pages also correspond to virtual addresses from [start, start + nr_pages) in __gup_longterm_locked.
For example, when __gup_longterm_locked() is returned, the following must be true: PHYSICAL VIRTUAL page_to_phys(pages[0]) -> start + 0 * PAGE_SIZE page_to_phys(pages[1]) -> start + 1 * PAGE_SIZE page_to_phys(pages[2]) -> start + 2 * PAGE_SIZE page_to_phys(pages[3]) -> start + 3 * PAGE_SIZE ... page_to_phys(pages[nr_pages - 1]) -> start + (nr_pages - 1) * PAGE_SIZE
If any pages[i] is part of a compound page (i.e. huge page), we can't have other pages to be in the middle of that page in the array..
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
Hmm, OK, but I'm not sure that is great either
I will send out a new series. We can discuss it there if you have suggestions for improvement here.
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.
However the page can't be unpinned until it is put on the LRU (and I'm hoping that the LRU is enough of a 'lock' to make that safe, no idea)
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.
I don't like this at all :( Lots of stuff relies on GUP, introducing a random flakiness like this not good.
This is actually standard migration procedure, elsewhere in the kernel we migrate pages in exactly the same fashion: isolate and later migrate. The isolation works for LRU only pages.
Jason