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.
You can't skip pages in the pages[] array under the assumption they are contiguous. ie the i+=step is wrong.
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
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.
Jason