Messages in this thread Patch in this message | | | Date | Thu, 3 Feb 2022 13:19:33 -0800 | Subject | Re: [PATCH v3 2/4] mm/gup: clean up follow_pfn_pte() slightly | From | John Hubbard <> |
| |
On 2/3/22 07:18, Matthew Wilcox wrote: ... >>> This will also modify the error code returned from follow_page(). >> >> Er, but isn't that the whole point of this entire design? It is what >> the commit that added it says: >> >> commit 1027e4436b6a5c413c95d95e50d0f26348a602ac >> Author: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> Date: Fri Sep 4 15:47:55 2015 -0700 >> >> mm: make GUP handle pfn mapping unless FOLL_GET is requested >> >> With DAX, pfn mapping becoming more common. The patch adjusts GUP code to >> cover pfn mapping for cases when we don't need struct page to proceed. >> >> To make it possible, let's change follow_page() code to return -EEXIST >> error code if proper page table entry exists, but no corresponding struct >> page. __get_user_page() would ignore the error code and move to the next >> page frame. >> >> The immediate effect of the change is working MAP_POPULATE and mlock() on >> DAX mappings. >> >>> A quick audit shows that at least the user in mm/migrate.c will >>> propagate this error code to userspace and I'm not sure the change >>> in error code will not break something... EEXIST is a bit strange >>> error code to get from move_pages(2). >> >> That makes sense, maybe move_pages should squash the return codes to >> EEXIST? > > I think EFAULT is the closest: > This is a zero page or the memory area is not mapped by the > process. > > EBUSY implies it can be tried again later. >
OK. I definitely need to rework the commit description now, but the diffs are looking like this:
diff --git a/mm/gup.c b/mm/gup.c index 65575ae3602f..cad3f28492e3 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -439,10 +439,6 @@ static struct page *no_page_table(struct vm_area_struct *vma, static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address, pte_t *pte, unsigned int flags) { - /* No page to get reference */ - if (flags & (FOLL_GET | FOLL_PIN)) - return -EFAULT; - if (flags & FOLL_TOUCH) { pte_t entry = *pte;
@@ -1180,8 +1176,13 @@ static long __get_user_pages(struct mm_struct *mm, } else if (PTR_ERR(page) == -EEXIST) { /* * Proper page table entry exists, but no corresponding - * struct page. + * struct page. If the caller expects **pages to be + * filled in, bail out now, because that can't be done + * for this page. */ + if (pages) + goto out; + goto next_page; } else if (IS_ERR(page)) { ret = PTR_ERR(page); diff --git a/mm/migrate.c b/mm/migrate.c index c7da064b4781..be0d5ae36dc1 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1761,6 +1761,13 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, continue; }
+ /* + * The move_pages() man page does not have an -EEXIST choice, so + * use -EFAULT instead. + */ + if (err == -EEXIST) + err = -EFAULT; + /* * If the page is already on the target node (!err), store the * node, otherwise, store the err. thanks, -- John Hubbard NVIDIA
| |