On Tue, Nov 12, 2019 at 2:24 PM John Hubbard jhubbard@nvidia.com wrote:
On 11/12/19 1:57 PM, Dan Williams wrote: ...
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index d864277ea16f..017689b7c32b 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -348,24 +348,20 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, flags |= FOLL_WRITE;
down_read(&mm->mmap_sem);
if (mm == current->mm) {ret = get_user_pages(vaddr, 1, flags | FOLL_LONGTERM, page,vmas);} else {ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page,vmas, NULL);/** The lifetime of a vaddr_get_pfn() page pin is* userspace-controlled. In the fs-dax case this could* lead to indefinite stalls in filesystem operations.* Disallow attempts to pin fs-dax pages via this* interface.*/if (ret > 0 && vma_is_fsdax(vmas[0])) {ret = -EOPNOTSUPP;put_page(page[0]);}
ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,page, vmas, NULL);Hmm, what's the point of passing FOLL_LONGTERM to get_user_pages_remote() if get_user_pages_remote() is not going to check the vma? I think we got to this code state because the
FOLL_LONGTERM is short-lived in this location, because patch 23 ("mm/gup: remove support for gup(FOLL_LONGTERM)") removes it, after callers are changed over to pin_longterm_pages*().
So FOLL_LONGTERM is not doing much now, but it is basically a marker for "change gup(FOLL_LONGTERM) to pin_longterm_pages()", and patch 18 actually makes that change.
And then pin_longterm_pages*() is, in turn, a way to mark all the places that need file system and/or user space interactions (layout leases, etc), as per "Case 2: RDMA" in the new Documentation/vm/pin_user_pages.rst.
Ah, sorry. This was the first time I had looked at this series and jumped in without reading the background.
Your patch as is looks ok, I assume you've removed the FOLL_LONGTERM warning in get_user_pages_remote in another patch?
get_user_pages() vs get_user_pages_remote() split predated the introduction of FOLL_LONGTERM.
Yes. And I do want clean this up as I go, so we don't end up with stale concepts lingering in gup.c...
I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) && vma_is_fsdax()) check and that would also remove the need for __gup_longterm_locked.
Good idea, but there is still the call to check_and_migrate_cma_pages(), inside __gup_longterm_locked(). So it's a little more involved and we can't trivially delete __gup_longterm_locked() yet, right?
[ add Aneesh ]
Yes, you're right. I had overlooked that had snuck in there. That to me similarly needs to be pushed down into the core with its own FOLL flag, or it needs to be an explicit fixup that each caller does after get_user_pages. The fact that migration silently happens as a side effect of gup is too magical for my taste.