[PATCH 1/1] vfio/spapr_tce: convert get_user_pages() --> pin_user_pages()

[PATCH 1/1] vfio/spapr_tce: convert get_user_pages() --> pin_user_pages()

[John Hubbard]

This code was using get_user_pages*(), in a "Case 2" scenario
(DMA/RDMA), using the categorization from [1]. That means that it's
time to convert the get_user_pages*() + put_page() calls to
pin_user_pages*() + unpin_user_pages() calls.

There is some helpful background in [2]: basically, this is a small
part of fixing a long-standing disconnect between pinning pages, and
file systems' use of those pages.

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
    https://lwn.net/Articles/807108/

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---

Hi,

I'm compile-tested this, but am not able to run-time test, so any
testing help is much appreciated!

thanks,
John Hubbard
NVIDIA

 drivers/vfio/vfio_iommu_spapr_tce.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 16b3adc508db..fe888b5dcc00 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -383,7 +383,7 @@ static void tce_iommu_unuse_page(struct tce_container *container,
 	struct page *page;
 
 	page = pfn_to_page(hpa >> PAGE_SHIFT);
-	put_page(page);
+	unpin_user_page(page);
 }
 
 static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container,
@@ -486,7 +486,7 @@ static int tce_iommu_use_page(unsigned long tce, unsigned long *hpa)
 	struct page *page = NULL;
 	enum dma_data_direction direction = iommu_tce_direction(tce);
 
-	if (get_user_pages_fast(tce & PAGE_MASK, 1,
+	if (pin_user_pages_fast(tce & PAGE_MASK, 1,
 			direction != DMA_TO_DEVICE ? FOLL_WRITE : 0,
 			&page) != 1)
 		return -EFAULT;
-- 
2.26.2

Re: [PATCH 1/1] vfio/spapr_tce: convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

Hi John,

On Sat, May 23, 2020 at 7:13 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> This code was using get_user_pages*(), in a "Case 2" scenario
> (DMA/RDMA), using the categorization from [1]. That means that it's
> time to convert the get_user_pages*() + put_page() calls to
> pin_user_pages*() + unpin_user_pages() calls.
>
> There is some helpful background in [2]: basically, this is a small
> part of fixing a long-standing disconnect between pinning pages, and
> file systems' use of those pages.
>
> [1] Documentation/core-api/pin_user_pages.rst
>
> [2] "Explicit pinning of user-space pages":
>     https://lwn.net/Articles/807108/
>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>
> Hi,
>
> I'm compile-tested this, but am not able to run-time test, so any
> testing help is much appreciated!
>
> thanks,
> John Hubbard
> NVIDIA
>
>  drivers/vfio/vfio_iommu_spapr_tce.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 16b3adc508db..fe888b5dcc00 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -383,7 +383,7 @@ static void tce_iommu_unuse_page(struct tce_container *container,
>         struct page *page;
>
>         page = pfn_to_page(hpa >> PAGE_SHIFT);
> -       put_page(page);
> +       unpin_user_page(page);
>  }
>
>  static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container,
> @@ -486,7 +486,7 @@ static int tce_iommu_use_page(unsigned long tce, unsigned long *hpa)
>         struct page *page = NULL;
>         enum dma_data_direction direction = iommu_tce_direction(tce);
>
> -       if (get_user_pages_fast(tce & PAGE_MASK, 1,
> +       if (pin_user_pages_fast(tce & PAGE_MASK, 1,
>                         direction != DMA_TO_DEVICE ? FOLL_WRITE : 0,
>                         &page) != 1)
>                 return -EFAULT;

There are few places where nr_pages is passed as 1 to get_user_pages_fast().
With similar conversion those will be changed to pin_user_pages_fast().

Does it make sense to add an inline like - pin_user_page_fast(), similar to
get_user_page_fast_only() ( now merged in linux-next) ?

Re: [PATCH 1/1] vfio/spapr_tce: convert get_user_pages() --> pin_user_pages()

[John Hubbard]

On 2020-05-26 12:28, Souptick Joarder wrote:
>> @@ -486,7 +486,7 @@ static int tce_iommu_use_page(unsigned long tce, unsigned long *hpa)
>>          struct page *page = NULL;
>>          enum dma_data_direction direction = iommu_tce_direction(tce);
>>
>> -       if (get_user_pages_fast(tce & PAGE_MASK, 1,
>> +       if (pin_user_pages_fast(tce & PAGE_MASK, 1,
>>                          direction != DMA_TO_DEVICE ? FOLL_WRITE : 0,
>>                          &page) != 1)
>>                  return -EFAULT;
> 
> There are few places where nr_pages is passed as 1 to get_user_pages_fast().
> With similar conversion those will be changed to pin_user_pages_fast().
> 
> Does it make sense to add an inline like - pin_user_page_fast(), similar to
> get_user_page_fast_only() ( now merged in linux-next) ?
> 

Perhaps not *just* yet, IMHO. There are only two places so far: here, and
dax_lock_page(). And we don't expect that many places, either, because
pin_user_pages*(), unlike get_user_pages(), is more likely to operate on
a bunch of pages at once. Although, that could change if we look into the
remaining call sites and find more single-page cases that need a gup-to-pup
conversion.

get_user_pages*() has a few more situations (Case 4, in
Documentation/core-api/pin_user_pages.rst: struct page manipulation) in
which it operates on single pages. Those will remain get_user_pages*()
calls, or perhaps change to get_user_page().


thanks,
-- 
John Hubbard
NVIDIA