On 11/4/19 12:37 PM, Jason Gunthorpe wrote:
On Mon, Nov 04, 2019 at 03:31:53PM -0500, Jerome Glisse wrote:
Note for Jason: the (a) or (b) items are talking about the vfio case, which is one of the two call sites that now use pin_longterm_pages_remote(), and the other one is infiniband:
drivers/infiniband/core/umem_odp.c:646: npages = pin_longterm_pages_remote(owning_process, owning_mm, drivers/vfio/vfio_iommu_type1.c:353: ret = pin_longterm_pages_remote(NULL, mm, vaddr, 1,
vfio should be reverted until it can be properly implemented. The issue is that when you fix the implementation you might break vfio existing user and thus regress the kernel from user point of view. So i rather have the change to vfio reverted, i believe it was not well understood when it got upstream, between in my 5.4 tree it is still gup_remote not longterm.
It is clearly a bug, vfio must use LONGTERM, and does right above this remote call:
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);I'm not even sure that it really makes any sense to build a 'if' like that, surely just always call remote??
Right, and I thought about this when converting, and realized that the above code is working around the current gup.c limitations, which are "cannot support gup remote with FOLL_LONGTERM".
Given that observation, the code is getting itself some FOLL_LONGTERM support for the non-remote case, and only hitting the limitation if the mm really is non-current.
And if you look at my patch, it keeps the same behavior, while adding in the new wrapper calls.
So...thoughts, preferences?
thanks,
John Hubbard NVIDIA