[PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[xuxiaoyang (C)]

vfio_iommu_type1_pin_pages is very inefficient because
it is processed page by page when calling vfio_pin_page_external.
Added contiguous_vaddr_get_pfn to process continuous pages
to reduce the number of loops, thereby improving performance.

Signed-off-by: Xiaoyang Xu <xuxiaoyang2@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 241 ++++++++++++++++++++++++++++----
 1 file changed, 214 insertions(+), 27 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 67e827638995..935f80807527 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -628,6 +628,206 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
 	return unlocked;
 }

+static int contiguous_vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
+				    int prot, long npage, unsigned long *phys_pfn)
+{
+	struct page **pages = NULL;
+	unsigned int flags = 0;
+	int i, ret;
+
+	pages = kvmalloc_array(npage, sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		return -ENOMEM;
+
+	if (prot & IOMMU_WRITE)
+		flags |= FOLL_WRITE;
+
+	mmap_read_lock(mm);
+	ret = pin_user_pages_remote(mm, vaddr, npage, flags | FOLL_LONGTERM,
+				    pages, NULL, NULL);
+	mmap_read_unlock(mm);
+
+	for (i = 0; i < ret; i++)
+		*(phys_pfn + i) = page_to_pfn(pages[i]);
+
+	kvfree(pages);
+
+	return ret;
+}
+
+static int vfio_pin_contiguous_pages_external(struct vfio_iommu *iommu,
+				    struct vfio_dma *dma,
+				    unsigned long *user_pfn,
+				    int npage, unsigned long *phys_pfn,
+				    bool do_accounting)
+{
+	int ret, i, j, lock_acct = 0;
+	unsigned long remote_vaddr;
+	dma_addr_t iova;
+	struct mm_struct *mm;
+	struct vfio_pfn *vpfn;
+
+	mm = get_task_mm(dma->task);
+	if (!mm)
+		return -ENODEV;
+
+	iova = user_pfn[0] << PAGE_SHIFT;
+	remote_vaddr = dma->vaddr + iova - dma->iova;
+	ret = contiguous_vaddr_get_pfn(mm, remote_vaddr, dma->prot,
+					    npage, phys_pfn);
+	mmput(mm);
+	if (ret <= 0)
+		return ret;
+
+	npage = ret;
+	for (i = 0; i < npage; i++) {
+		iova = user_pfn[i] << PAGE_SHIFT;
+		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
+		if (ret)
+			goto unwind;
+
+		if (!is_invalid_reserved_pfn(phys_pfn[i]))
+			lock_acct++;
+
+		if (iommu->dirty_page_tracking) {
+			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
+
+			/*
+			 * Bitmap populated with the smallest supported page
+			 * size
+			 */
+			bitmap_set(dma->bitmap,
+				   (iova - dma->iova) >> pgshift, 1);
+		}
+	}
+
+	if (do_accounting) {
+		ret = vfio_lock_acct(dma, lock_acct, true);
+		if (ret) {
+			if (ret == -ENOMEM)
+				pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
+					__func__, dma->task->comm, task_pid_nr(dma->task),
+					task_rlimit(dma->task, RLIMIT_MEMLOCK));
+			goto unwind;
+		}
+	}
+
+	return i;
+unwind:
+	for (j = 0; j < npage; j++) {
+		put_pfn(phys_pfn[j], dma->prot);
+		phys_pfn[j] = 0;
+	}
+
+	for (j = 0; j < i; j++) {
+		iova = user_pfn[j] << PAGE_SHIFT;
+		vpfn = vfio_find_vpfn(dma, iova);
+		if (vpfn)
+			vfio_remove_from_pfn_list(dma, vpfn);
+	}
+
+	return ret;
+}
+
+static int vfio_iommu_type1_pin_contiguous_pages(struct vfio_iommu *iommu,
+					    struct vfio_dma *dma,
+					    unsigned long *user_pfn,
+					    int npage, unsigned long *phys_pfn,
+					    bool do_accounting)
+{
+	int ret, i, j;
+	unsigned long remote_vaddr;
+	dma_addr_t iova;
+
+	ret = vfio_pin_contiguous_pages_external(iommu, dma, user_pfn, npage,
+				phys_pfn, do_accounting);
+	if (ret == npage)
+		return ret;
+
+	if (ret < 0)
+		ret = 0;
+
+	for (i = ret; i < npage; i++) {
+		iova = user_pfn[i] << PAGE_SHIFT;
+		remote_vaddr = dma->vaddr + iova - dma->iova;
+
+		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
+			    do_accounting);
+		if (ret)
+			goto pin_unwind;
+
+		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
+		if (ret) {
+			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
+				vfio_lock_acct(dma, -1, true);
+			goto pin_unwind;
+		}
+
+		if (iommu->dirty_page_tracking) {
+			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
+
+			/*
+			 * Bitmap populated with the smallest supported page
+			 * size
+			 */
+			bitmap_set(dma->bitmap,
+					   (iova - dma->iova) >> pgshift, 1);
+		}
+	}
+
+	return i;
+
+pin_unwind:
+	phys_pfn[i] = 0;
+	for (j = 0; j < i; j++) {
+		dma_addr_t iova;
+
+		iova = user_pfn[j] << PAGE_SHIFT;
+		vfio_unpin_page_external(dma, iova, do_accounting);
+		phys_pfn[j] = 0;
+	}
+
+	return ret;
+}
+
+static int vfio_iommu_type1_get_contiguous_pages_length(struct vfio_iommu *iommu,
+				    unsigned long *user_pfn, int npage, int prot)
+{
+	struct vfio_dma *dma_base;
+	int i;
+	dma_addr_t iova;
+	struct vfio_pfn *vpfn;
+
+	if (npage <= 1)
+		return npage;
+
+	iova = user_pfn[0] << PAGE_SHIFT;
+	dma_base = vfio_find_dma(iommu, iova, PAGE_SIZE);
+	if (!dma_base)
+		return -EINVAL;
+
+	if ((dma_base->prot & prot) != prot)
+		return -EPERM;
+
+	for (i = 1; i < npage; i++) {
+		iova = user_pfn[i] << PAGE_SHIFT;
+
+		if (iova >= dma_base->iova + dma_base->size ||
+				iova + PAGE_SIZE <= dma_base->iova)
+			break;
+
+		vpfn = vfio_iova_get_vfio_pfn(dma_base, iova);
+		if (vpfn) {
+			vfio_iova_put_vfio_pfn(dma_base, vpfn);
+			break;
+		}
+
+		if (user_pfn[i] != user_pfn[0] + i)
+			break;
+	}
+	return i;
+}
+
 static int vfio_iommu_type1_pin_pages(void *iommu_data,
 				      struct iommu_group *iommu_group,
 				      unsigned long *user_pfn,
@@ -637,9 +837,9 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_group *group;
 	int i, j, ret;
-	unsigned long remote_vaddr;
 	struct vfio_dma *dma;
 	bool do_accounting;
+	int contiguous_npage;

 	if (!iommu || !user_pfn || !phys_pfn)
 		return -EINVAL;
@@ -663,7 +863,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 	 */
 	do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);

-	for (i = 0; i < npage; i++) {
+	for (i = 0; i < npage; i += contiguous_npage) {
 		dma_addr_t iova;
 		struct vfio_pfn *vpfn;

@@ -682,31 +882,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 		vpfn = vfio_iova_get_vfio_pfn(dma, iova);
 		if (vpfn) {
 			phys_pfn[i] = vpfn->pfn;
-			continue;
-		}
-
-		remote_vaddr = dma->vaddr + (iova - dma->iova);
-		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
-					     do_accounting);
-		if (ret)
-			goto pin_unwind;
-
-		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
-		if (ret) {
-			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
-				vfio_lock_acct(dma, -1, true);
-			goto pin_unwind;
-		}
-
-		if (iommu->dirty_page_tracking) {
-			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
-
-			/*
-			 * Bitmap populated with the smallest supported page
-			 * size
-			 */
-			bitmap_set(dma->bitmap,
-				   (iova - dma->iova) >> pgshift, 1);
+			contiguous_npage = 1;
+		} else {
+			ret = vfio_iommu_type1_get_contiguous_pages_length(iommu,
+					&user_pfn[i], npage - i, prot);
+			if (ret < 0)
+				goto pin_unwind;
+
+			ret = vfio_iommu_type1_pin_contiguous_pages(iommu,
+					dma, &user_pfn[i], ret, &phys_pfn[i], do_accounting);
+			if (ret < 0)
+				goto pin_unwind;
+			contiguous_npage = ret;
 		}
 	}
 	ret = i;
--
2.19.1

Re: [PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[Alex Williamson]

On Tue, 10 Nov 2020 21:42:33 +0800
"xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:

> vfio_iommu_type1_pin_pages is very inefficient because
> it is processed page by page when calling vfio_pin_page_external.
> Added contiguous_vaddr_get_pfn to process continuous pages
> to reduce the number of loops, thereby improving performance.

vfio_pin_pages() accepts an array of unrelated iova pfns and processes
each to return the physical pfn.  AFAICT this proposal makes an
unfounded and unverified assumption that the caller is asking for a
range of contiguous iova pfns.  That's not the semantics of the call.
This is wrong.  Thanks,

Alex

Re: [PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[xuxiaoyang (C)]

On 2020/11/11 23:56, Alex Williamson wrote:
> On Tue, 10 Nov 2020 21:42:33 +0800
> "xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:
> 
>> vfio_iommu_type1_pin_pages is very inefficient because
>> it is processed page by page when calling vfio_pin_page_external.
>> Added contiguous_vaddr_get_pfn to process continuous pages
>> to reduce the number of loops, thereby improving performance.
> 
> vfio_pin_pages() accepts an array of unrelated iova pfns and processes
> each to return the physical pfn.  AFAICT this proposal makes an
> unfounded and unverified assumption that the caller is asking for a
> range of contiguous iova pfns.  That's not the semantics of the call.
> This is wrong.  Thanks,
> 
> Alex
> 
> .
> 
Thank you for your reply.  Sorry that the comment is too simple
and not clear enough.
We did not change the external behavior of the function.  What we
have to do is to divide the iova pfn array into multiple continuous
ranges and optimize them.  For example, when the iova pfn array is
{1,5,6,7,9}, it will be divided into three groups {1}, {5,6,7}, {9}
for processing.  When processing {5,6,7}, the number of calls to
pin_user_pages_remote is reduced from 3 times to once.  The more
continuous the iova pfn array, the greater the performance improvement.
I see that most of the iova pfn arrays processed by callers are
continuous, such as pfn_array_pin, gvt_pin_guest_page.  For them,
performance will be improved.

Regards,
Xu

Re: [PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[Alex Williamson]

On Tue, 10 Nov 2020 21:42:33 +0800
"xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:

> vfio_iommu_type1_pin_pages is very inefficient because
> it is processed page by page when calling vfio_pin_page_external.
> Added contiguous_vaddr_get_pfn to process continuous pages
> to reduce the number of loops, thereby improving performance.
> 
> Signed-off-by: Xiaoyang Xu <xuxiaoyang2@huawei.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 241 ++++++++++++++++++++++++++++----
>  1 file changed, 214 insertions(+), 27 deletions(-)

Sorry for my previous misunderstanding of the logic here.  Still, this
adds a lot of complexity, can you quantify the performance improvement
you're seeing?  Would it instead be more worthwhile to support an
interface that pins based on an iova and range?  Further comments below.

> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 67e827638995..935f80807527 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -628,6 +628,206 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
>  	return unlocked;
>  }
> 
> +static int contiguous_vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
> +				    int prot, long npage, unsigned long *phys_pfn)
> +{
> +	struct page **pages = NULL;
> +	unsigned int flags = 0;
> +	int i, ret;
> +
> +	pages = kvmalloc_array(npage, sizeof(struct page *), GFP_KERNEL);
> +	if (!pages)
> +		return -ENOMEM;
> +
> +	if (prot & IOMMU_WRITE)
> +		flags |= FOLL_WRITE;
> +
> +	mmap_read_lock(mm);
> +	ret = pin_user_pages_remote(mm, vaddr, npage, flags | FOLL_LONGTERM,
> +				    pages, NULL, NULL);

This is essentially the root of the performance improvement claim,
right?  ie. we're pinning a range of pages rather than individually.
Unfortunately it means we also add a dynamic memory allocation even
when npage=1.


> +	mmap_read_unlock(mm);
> +
> +	for (i = 0; i < ret; i++)
> +		*(phys_pfn + i) = page_to_pfn(pages[i]);
> +
> +	kvfree(pages);
> +
> +	return ret;
> +}
> +
> +static int vfio_pin_contiguous_pages_external(struct vfio_iommu *iommu,
> +				    struct vfio_dma *dma,
> +				    unsigned long *user_pfn,
> +				    int npage, unsigned long *phys_pfn,
> +				    bool do_accounting)
> +{
> +	int ret, i, j, lock_acct = 0;
> +	unsigned long remote_vaddr;
> +	dma_addr_t iova;
> +	struct mm_struct *mm;
> +	struct vfio_pfn *vpfn;
> +
> +	mm = get_task_mm(dma->task);
> +	if (!mm)
> +		return -ENODEV;
> +
> +	iova = user_pfn[0] << PAGE_SHIFT;
> +	remote_vaddr = dma->vaddr + iova - dma->iova;
> +	ret = contiguous_vaddr_get_pfn(mm, remote_vaddr, dma->prot,
> +					    npage, phys_pfn);
> +	mmput(mm);
> +	if (ret <= 0)
> +		return ret;
> +
> +	npage = ret;
> +	for (i = 0; i < npage; i++) {
> +		iova = user_pfn[i] << PAGE_SHIFT;
> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> +		if (ret)
> +			goto unwind;
> +
> +		if (!is_invalid_reserved_pfn(phys_pfn[i]))
> +			lock_acct++;
> +
> +		if (iommu->dirty_page_tracking) {
> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> +
> +			/*
> +			 * Bitmap populated with the smallest supported page
> +			 * size
> +			 */
> +			bitmap_set(dma->bitmap,
> +				   (iova - dma->iova) >> pgshift, 1);
> +		}

It doesn't make sense that we wouldn't also optimize this to simply set
npage bits.  There's also no unwind for this.

> +	}
> +
> +	if (do_accounting) {
> +		ret = vfio_lock_acct(dma, lock_acct, true);
> +		if (ret) {
> +			if (ret == -ENOMEM)
> +				pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
> +					__func__, dma->task->comm, task_pid_nr(dma->task),
> +					task_rlimit(dma->task, RLIMIT_MEMLOCK));
> +			goto unwind;
> +		}
> +	}

This algorithm allows pinning many more pages in advance of testing
whether we've exceeded the task locked memory limit than the per page
approach.


> +
> +	return i;
> +unwind:
> +	for (j = 0; j < npage; j++) {
> +		put_pfn(phys_pfn[j], dma->prot);
> +		phys_pfn[j] = 0;
> +	}
> +
> +	for (j = 0; j < i; j++) {
> +		iova = user_pfn[j] << PAGE_SHIFT;
> +		vpfn = vfio_find_vpfn(dma, iova);
> +		if (vpfn)

Seems like not finding a vpfn would be an error.

> +			vfio_remove_from_pfn_list(dma, vpfn);

It seems poor form not to use vfio_iova_put_vfio_pfn() here even if we
know we hold the only reference.


> +	}
> +
> +	return ret;
> +}
> +
> +static int vfio_iommu_type1_pin_contiguous_pages(struct vfio_iommu *iommu,
> +					    struct vfio_dma *dma,
> +					    unsigned long *user_pfn,
> +					    int npage, unsigned long *phys_pfn,
> +					    bool do_accounting)
> +{
> +	int ret, i, j;
> +	unsigned long remote_vaddr;
> +	dma_addr_t iova;
> +
> +	ret = vfio_pin_contiguous_pages_external(iommu, dma, user_pfn, npage,
> +				phys_pfn, do_accounting);
> +	if (ret == npage)
> +		return ret;
> +
> +	if (ret < 0)
> +		ret = 0;


I'm lost, why do we need the single page iteration below??

> +	for (i = ret; i < npage; i++) {
> +		iova = user_pfn[i] << PAGE_SHIFT;
> +		remote_vaddr = dma->vaddr + iova - dma->iova;
> +
> +		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
> +			    do_accounting);
> +		if (ret)
> +			goto pin_unwind;
> +
> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> +		if (ret) {
> +			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
> +				vfio_lock_acct(dma, -1, true);
> +			goto pin_unwind;
> +		}
> +
> +		if (iommu->dirty_page_tracking) {
> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> +
> +			/*
> +			 * Bitmap populated with the smallest supported page
> +			 * size
> +			 */
> +			bitmap_set(dma->bitmap,
> +					   (iova - dma->iova) >> pgshift, 1);
> +		}
> +	}
> +
> +	return i;
> +
> +pin_unwind:
> +	phys_pfn[i] = 0;
> +	for (j = 0; j < i; j++) {
> +		dma_addr_t iova;
> +
> +		iova = user_pfn[j] << PAGE_SHIFT;
> +		vfio_unpin_page_external(dma, iova, do_accounting);
> +		phys_pfn[j] = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static int vfio_iommu_type1_get_contiguous_pages_length(struct vfio_iommu *iommu,
> +				    unsigned long *user_pfn, int npage, int prot)
> +{
> +	struct vfio_dma *dma_base;
> +	int i;
> +	dma_addr_t iova;
> +	struct vfio_pfn *vpfn;
> +
> +	if (npage <= 1)
> +		return npage;
> +
> +	iova = user_pfn[0] << PAGE_SHIFT;
> +	dma_base = vfio_find_dma(iommu, iova, PAGE_SIZE);
> +	if (!dma_base)
> +		return -EINVAL;
> +
> +	if ((dma_base->prot & prot) != prot)
> +		return -EPERM;
> +
> +	for (i = 1; i < npage; i++) {
> +		iova = user_pfn[i] << PAGE_SHIFT;
> +
> +		if (iova >= dma_base->iova + dma_base->size ||
> +				iova + PAGE_SIZE <= dma_base->iova)
> +			break;
> +
> +		vpfn = vfio_iova_get_vfio_pfn(dma_base, iova);
> +		if (vpfn) {
> +			vfio_iova_put_vfio_pfn(dma_base, vpfn);

Why not just use vfio_find_vpfn() rather than get+put?

> +			break;
> +		}
> +
> +		if (user_pfn[i] != user_pfn[0] + i)

Shouldn't this be the first test?

> +			break;
> +	}
> +	return i;
> +}
> +
>  static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  				      struct iommu_group *iommu_group,
>  				      unsigned long *user_pfn,
> @@ -637,9 +837,9 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  	struct vfio_iommu *iommu = iommu_data;
>  	struct vfio_group *group;
>  	int i, j, ret;
> -	unsigned long remote_vaddr;
>  	struct vfio_dma *dma;
>  	bool do_accounting;
> +	int contiguous_npage;
> 
>  	if (!iommu || !user_pfn || !phys_pfn)
>  		return -EINVAL;
> @@ -663,7 +863,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  	 */
>  	do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
> 
> -	for (i = 0; i < npage; i++) {
> +	for (i = 0; i < npage; i += contiguous_npage) {
>  		dma_addr_t iova;
>  		struct vfio_pfn *vpfn;
> 
> @@ -682,31 +882,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  		vpfn = vfio_iova_get_vfio_pfn(dma, iova);
>  		if (vpfn) {
>  			phys_pfn[i] = vpfn->pfn;
> -			continue;
> -		}
> -
> -		remote_vaddr = dma->vaddr + (iova - dma->iova);
> -		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
> -					     do_accounting);
> -		if (ret)
> -			goto pin_unwind;
> -
> -		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> -		if (ret) {
> -			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
> -				vfio_lock_acct(dma, -1, true);
> -			goto pin_unwind;
> -		}
> -
> -		if (iommu->dirty_page_tracking) {
> -			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> -
> -			/*
> -			 * Bitmap populated with the smallest supported page
> -			 * size
> -			 */
> -			bitmap_set(dma->bitmap,
> -				   (iova - dma->iova) >> pgshift, 1);
> +			contiguous_npage = 1;
> +		} else {
> +			ret = vfio_iommu_type1_get_contiguous_pages_length(iommu,
> +					&user_pfn[i], npage - i, prot);


It doesn't make a lot of sense to me that this isn't more integrated
into the base function.  For example, we're passing &user_pfn[i] for
which we've already converted to an iova, found the vfio_dma associated
to that iova, and checked the protection.  This callout does all of
that again on the same.

> +			if (ret < 0)
> +				goto pin_unwind;
> +
> +			ret = vfio_iommu_type1_pin_contiguous_pages(iommu,
> +					dma, &user_pfn[i], ret, &phys_pfn[i], do_accounting);
> +			if (ret < 0)
> +				goto pin_unwind;
> +			contiguous_npage = ret;
>  		}
>  	}
>  	ret = i;


This all seems _way_ more complicated than it needs to be, there are
too many different ways to flow through this code and claims of a
performance improvement are not substantiated with evidence.  The type1
code is already too fragile.  Please simplify and justify.  Thanks,

Alex

Re: [PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[xuxiaoyang (C)]

On 2020/11/14 0:44, Alex Williamson wrote:
> On Tue, 10 Nov 2020 21:42:33 +0800
> "xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:
> 
>> vfio_iommu_type1_pin_pages is very inefficient because
>> it is processed page by page when calling vfio_pin_page_external.
>> Added contiguous_vaddr_get_pfn to process continuous pages
>> to reduce the number of loops, thereby improving performance.
>>
>> Signed-off-by: Xiaoyang Xu <xuxiaoyang2@huawei.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 241 ++++++++++++++++++++++++++++----
>>  1 file changed, 214 insertions(+), 27 deletions(-)
> 
> Sorry for my previous misunderstanding of the logic here.  Still, this
> adds a lot of complexity, can you quantify the performance improvement
> you're seeing?  Would it instead be more worthwhile to support an
> interface that pins based on an iova and range?  Further comments below.
> 
Thank you for your reply.  I have a set of performance test data for reference.
The host kernel version of my test environment is 4.19.36, and the test cases
are for pin one page and 512 pages.  When pin 512pages, the input is a
continuous iova address.  At the same time increase the measurement factor of
whether the mem backend uses large pages.  The following is the average of
multiple tests.

The patch was not applied
                    1 page           512 pages
no huge pages：     1638ns           223651ns
THP：               1668ns           222330ns
HugeTLB：           1526ns           208151ns

The patch was applied
                    1 page           512 pages
no huge pages       1735ns           167286ns
THP：               1934ns           126900ns
HugeTLB：           1713ns           102188ns

The performance will be reduced when the page is fixed with a single pin,
while the page time of continuous pins can be reduced to half of the original.
If you have other suggestions for testing methods, please let me know.

>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 67e827638995..935f80807527 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -628,6 +628,206 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
>>  	return unlocked;
>>  }
>>
>> +static int contiguous_vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
>> +				    int prot, long npage, unsigned long *phys_pfn)
>> +{
>> +	struct page **pages = NULL;
>> +	unsigned int flags = 0;
>> +	int i, ret;
>> +
>> +	pages = kvmalloc_array(npage, sizeof(struct page *), GFP_KERNEL);
>> +	if (!pages)
>> +		return -ENOMEM;
>> +
>> +	if (prot & IOMMU_WRITE)
>> +		flags |= FOLL_WRITE;
>> +
>> +	mmap_read_lock(mm);
>> +	ret = pin_user_pages_remote(mm, vaddr, npage, flags | FOLL_LONGTERM,
>> +				    pages, NULL, NULL);
> 
> This is essentially the root of the performance improvement claim,
> right?  ie. we're pinning a range of pages rather than individually.
> Unfortunately it means we also add a dynamic memory allocation even
> when npage=1.
> 
Yes, when npage = 1, I should keep the previous scheme of the scene
and use local variables to save time in allocated memory
> 
>> +	mmap_read_unlock(mm);
>> +
>> +	for (i = 0; i < ret; i++)
>> +		*(phys_pfn + i) = page_to_pfn(pages[i]);
>> +
>> +	kvfree(pages);
>> +
>> +	return ret;
>> +}
>> +
>> +static int vfio_pin_contiguous_pages_external(struct vfio_iommu *iommu,
>> +				    struct vfio_dma *dma,
>> +				    unsigned long *user_pfn,
>> +				    int npage, unsigned long *phys_pfn,
>> +				    bool do_accounting)
>> +{
>> +	int ret, i, j, lock_acct = 0;
>> +	unsigned long remote_vaddr;
>> +	dma_addr_t iova;
>> +	struct mm_struct *mm;
>> +	struct vfio_pfn *vpfn;
>> +
>> +	mm = get_task_mm(dma->task);
>> +	if (!mm)
>> +		return -ENODEV;
>> +
>> +	iova = user_pfn[0] << PAGE_SHIFT;
>> +	remote_vaddr = dma->vaddr + iova - dma->iova;
>> +	ret = contiguous_vaddr_get_pfn(mm, remote_vaddr, dma->prot,
>> +					    npage, phys_pfn);
>> +	mmput(mm);
>> +	if (ret <= 0)
>> +		return ret;
>> +
>> +	npage = ret;
>> +	for (i = 0; i < npage; i++) {
>> +		iova = user_pfn[i] << PAGE_SHIFT;
>> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
>> +		if (ret)
>> +			goto unwind;
>> +
>> +		if (!is_invalid_reserved_pfn(phys_pfn[i]))
>> +			lock_acct++;
>> +
>> +		if (iommu->dirty_page_tracking) {
>> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
>> +
>> +			/*
>> +			 * Bitmap populated with the smallest supported page
>> +			 * size
>> +			 */
>> +			bitmap_set(dma->bitmap,
>> +				   (iova - dma->iova) >> pgshift, 1);
>> +		}
> 
> It doesn't make sense that we wouldn't also optimize this to simply set
> npage bits.  There's also no unwind for this.
> 
Thank you for your correction, I should set npage to simplify.
There is no unwind process on the current master branch.  Is this a bug?
>> +	}
>> +
>> +	if (do_accounting) {
>> +		ret = vfio_lock_acct(dma, lock_acct, true);
>> +		if (ret) {
>> +			if (ret == -ENOMEM)
>> +				pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
>> +					__func__, dma->task->comm, task_pid_nr(dma->task),
>> +					task_rlimit(dma->task, RLIMIT_MEMLOCK));
>> +			goto unwind;
>> +		}
>> +	}
> 
> This algorithm allows pinning many more pages in advance of testing
> whether we've exceeded the task locked memory limit than the per page
> approach.
> 
More than 1~VFIO_PIN_PAGES_MAX_ENTRIES. But after failure, all pinned
pages will be released. Is there a big impact here?
> 
>> +
>> +	return i;
>> +unwind:
>> +	for (j = 0; j < npage; j++) {
>> +		put_pfn(phys_pfn[j], dma->prot);
>> +		phys_pfn[j] = 0;
>> +	}
>> +
>> +	for (j = 0; j < i; j++) {
>> +		iova = user_pfn[j] << PAGE_SHIFT;
>> +		vpfn = vfio_find_vpfn(dma, iova);
>> +		if (vpfn)
> 
> Seems like not finding a vpfn would be an error.
> 
I think the above process can ensure that vpfn is not NULL
and delete this judgment.
>> +			vfio_remove_from_pfn_list(dma, vpfn);
> 
> It seems poor form not to use vfio_iova_put_vfio_pfn() here even if we
> know we hold the only reference.
> 
The logic here can be reduced to a loop.
When j < i, call vfio_iova_put_vfio_pfn.
> 
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int vfio_iommu_type1_pin_contiguous_pages(struct vfio_iommu *iommu,
>> +					    struct vfio_dma *dma,
>> +					    unsigned long *user_pfn,
>> +					    int npage, unsigned long *phys_pfn,
>> +					    bool do_accounting)
>> +{
>> +	int ret, i, j;
>> +	unsigned long remote_vaddr;
>> +	dma_addr_t iova;
>> +
>> +	ret = vfio_pin_contiguous_pages_external(iommu, dma, user_pfn, npage,
>> +				phys_pfn, do_accounting);
>> +	if (ret == npage)
>> +		return ret;
>> +
>> +	if (ret < 0)
>> +		ret = 0;
> 
> 
> I'm lost, why do we need the single page iteration below??
> 
Since there is no retry in contiguous_vaddr_get_pfn, oncean error occurs,
the remaining pages will be processed in a single page.
Is it better to increase retry in contiguous_vaddr_get_pfn?
>> +	for (i = ret; i < npage; i++) {
>> +		iova = user_pfn[i] << PAGE_SHIFT;
>> +		remote_vaddr = dma->vaddr + iova - dma->iova;
>> +
>> +		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
>> +			    do_accounting);
>> +		if (ret)
>> +			goto pin_unwind;
>> +
>> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
>> +		if (ret) {
>> +			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
>> +				vfio_lock_acct(dma, -1, true);
>> +			goto pin_unwind;
>> +		}
>> +
>> +		if (iommu->dirty_page_tracking) {
>> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
>> +
>> +			/*
>> +			 * Bitmap populated with the smallest supported page
>> +			 * size
>> +			 */
>> +			bitmap_set(dma->bitmap,
>> +					   (iova - dma->iova) >> pgshift, 1);
>> +		}
>> +	}
>> +
>> +	return i;
>> +
>> +pin_unwind:
>> +	phys_pfn[i] = 0;
>> +	for (j = 0; j < i; j++) {
>> +		dma_addr_t iova;
>> +
>> +		iova = user_pfn[j] << PAGE_SHIFT;
>> +		vfio_unpin_page_external(dma, iova, do_accounting);
>> +		phys_pfn[j] = 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int vfio_iommu_type1_get_contiguous_pages_length(struct vfio_iommu *iommu,
>> +				    unsigned long *user_pfn, int npage, int prot)
>> +{
>> +	struct vfio_dma *dma_base;
>> +	int i;
>> +	dma_addr_t iova;
>> +	struct vfio_pfn *vpfn;
>> +
>> +	if (npage <= 1)
>> +		return npage;
>> +
>> +	iova = user_pfn[0] << PAGE_SHIFT;
>> +	dma_base = vfio_find_dma(iommu, iova, PAGE_SIZE);
>> +	if (!dma_base)
>> +		return -EINVAL;
>> +
>> +	if ((dma_base->prot & prot) != prot)
>> +		return -EPERM;
>> +
>> +	for (i = 1; i < npage; i++) {
>> +		iova = user_pfn[i] << PAGE_SHIFT;
>> +
>> +		if (iova >= dma_base->iova + dma_base->size ||
>> +				iova + PAGE_SIZE <= dma_base->iova)
>> +			break;
>> +
>> +		vpfn = vfio_iova_get_vfio_pfn(dma_base, iova);
>> +		if (vpfn) {
>> +			vfio_iova_put_vfio_pfn(dma_base, vpfn);
> 
> Why not just use vfio_find_vpfn() rather than get+put?
> 
Thank you for your correction, I should use vfio_find_vpfn here.
>> +			break;
>> +		}
>> +
>> +		if (user_pfn[i] != user_pfn[0] + i)
> 
> Shouldn't this be the first test?
> 
Thank you for your correction, the least costly judgment should be
the first test.
>> +			break;
>> +	}
>> +	return i;
>> +}
>> +
>>  static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>  				      struct iommu_group *iommu_group,
>>  				      unsigned long *user_pfn,
>> @@ -637,9 +837,9 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>  	struct vfio_iommu *iommu = iommu_data;
>>  	struct vfio_group *group;
>>  	int i, j, ret;
>> -	unsigned long remote_vaddr;
>>  	struct vfio_dma *dma;
>>  	bool do_accounting;
>> +	int contiguous_npage;
>>
>>  	if (!iommu || !user_pfn || !phys_pfn)
>>  		return -EINVAL;
>> @@ -663,7 +863,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>  	 */
>>  	do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
>>
>> -	for (i = 0; i < npage; i++) {
>> +	for (i = 0; i < npage; i += contiguous_npage) {
>>  		dma_addr_t iova;
>>  		struct vfio_pfn *vpfn;
>>
>> @@ -682,31 +882,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>  		vpfn = vfio_iova_get_vfio_pfn(dma, iova);
>>  		if (vpfn) {
>>  			phys_pfn[i] = vpfn->pfn;
>> -			continue;
>> -		}
>> -
>> -		remote_vaddr = dma->vaddr + (iova - dma->iova);
>> -		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
>> -					     do_accounting);
>> -		if (ret)
>> -			goto pin_unwind;
>> -
>> -		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
>> -		if (ret) {
>> -			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
>> -				vfio_lock_acct(dma, -1, true);
>> -			goto pin_unwind;
>> -		}
>> -
>> -		if (iommu->dirty_page_tracking) {
>> -			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
>> -
>> -			/*
>> -			 * Bitmap populated with the smallest supported page
>> -			 * size
>> -			 */
>> -			bitmap_set(dma->bitmap,
>> -				   (iova - dma->iova) >> pgshift, 1);
>> +			contiguous_npage = 1;
>> +		} else {
>> +			ret = vfio_iommu_type1_get_contiguous_pages_length(iommu,
>> +					&user_pfn[i], npage - i, prot);
> 
> 
> It doesn't make a lot of sense to me that this isn't more integrated
> into the base function.  For example, we're passing &user_pfn[i] for
> which we've already converted to an iova, found the vfio_dma associated
> to that iova, and checked the protection.  This callout does all of
> that again on the same.
> 
Thanks for your correction, I will delete the redundant check in
vfio_iommu_type1_get_contiguous_pages_length and simplify the function to
static int vfio_get_contiguous_pages_length (struct vfio_dma * dma,
unsigned long * user_pfn, int npage)
>> +			if (ret < 0)
>> +				goto pin_unwind;
>> +
>> +			ret = vfio_iommu_type1_pin_contiguous_pages(iommu,
>> +					dma, &user_pfn[i], ret, &phys_pfn[i], do_accounting);
>> +			if (ret < 0)
>> +				goto pin_unwind;
>> +			contiguous_npage = ret;
>>  		}
>>  	}
>>  	ret = i;
> 
> 
> This all seems _way_ more complicated than it needs to be, there are
> too many different ways to flow through this code and claims of a
> performance improvement are not substantiated with evidence.  The type1
> code is already too fragile.  Please simplify and justify.  Thanks,
> 
> Alex
> 
> .
> 
The main issue is the balance between performance and complexity.
The test data has been given above, please tell me your opinion.

Regards,
Xu

Re: [PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[Alex Williamson]

On Mon, 16 Nov 2020 21:47:33 +0800
"xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:

> On 2020/11/14 0:44, Alex Williamson wrote:
> > On Tue, 10 Nov 2020 21:42:33 +0800
> > "xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:
> >   
> >> vfio_iommu_type1_pin_pages is very inefficient because
> >> it is processed page by page when calling vfio_pin_page_external.
> >> Added contiguous_vaddr_get_pfn to process continuous pages
> >> to reduce the number of loops, thereby improving performance.
> >>
> >> Signed-off-by: Xiaoyang Xu <xuxiaoyang2@huawei.com>
> >> ---
> >>  drivers/vfio/vfio_iommu_type1.c | 241 ++++++++++++++++++++++++++++----
> >>  1 file changed, 214 insertions(+), 27 deletions(-)  
> > 
> > Sorry for my previous misunderstanding of the logic here.  Still, this
> > adds a lot of complexity, can you quantify the performance improvement
> > you're seeing?  Would it instead be more worthwhile to support an
> > interface that pins based on an iova and range?  Further comments below.
> >   
> Thank you for your reply.  I have a set of performance test data for reference.
> The host kernel version of my test environment is 4.19.36, and the test cases
> are for pin one page and 512 pages.  When pin 512pages, the input is a
> continuous iova address.  At the same time increase the measurement factor of
> whether the mem backend uses large pages.  The following is the average of
> multiple tests.
> 
> The patch was not applied
>                     1 page           512 pages
> no huge pages：     1638ns           223651ns
> THP：               1668ns           222330ns
> HugeTLB：           1526ns           208151ns
> 
> The patch was applied
>                     1 page           512 pages
> no huge pages       1735ns           167286ns
> THP：               1934ns           126900ns
> HugeTLB：           1713ns           102188ns
> 
> The performance will be reduced when the page is fixed with a single pin,
> while the page time of continuous pins can be reduced to half of the original.
> If you have other suggestions for testing methods, please let me know.

These are artificial test results, which in fact show a performance
decrease for the single page use cases.  What do we see in typical real
world scenarios?  Do those real world scenarios tend more towards large
arrays of contiguous IOVAs or single pages, or large arrays of
discontiguous IOVAs?  What's the resulting change in device
performance?  Are pages pinned at a high enough frequency that pinning
latency results in a measurable benefit at the device or to the
overhead on the host?

> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 67e827638995..935f80807527 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -628,6 +628,206 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
> >>  	return unlocked;
> >>  }
> >>
> >> +static int contiguous_vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
> >> +				    int prot, long npage, unsigned long *phys_pfn)
> >> +{
> >> +	struct page **pages = NULL;
> >> +	unsigned int flags = 0;
> >> +	int i, ret;
> >> +
> >> +	pages = kvmalloc_array(npage, sizeof(struct page *), GFP_KERNEL);
> >> +	if (!pages)
> >> +		return -ENOMEM;
> >> +
> >> +	if (prot & IOMMU_WRITE)
> >> +		flags |= FOLL_WRITE;
> >> +
> >> +	mmap_read_lock(mm);
> >> +	ret = pin_user_pages_remote(mm, vaddr, npage, flags | FOLL_LONGTERM,
> >> +				    pages, NULL, NULL);  
> > 
> > This is essentially the root of the performance improvement claim,
> > right?  ie. we're pinning a range of pages rather than individually.
> > Unfortunately it means we also add a dynamic memory allocation even
> > when npage=1.
> >   
> Yes, when npage = 1, I should keep the previous scheme of the scene
> and use local variables to save time in allocated memory
> >   
> >> +	mmap_read_unlock(mm);
> >> +
> >> +	for (i = 0; i < ret; i++)
> >> +		*(phys_pfn + i) = page_to_pfn(pages[i]);
> >> +
> >> +	kvfree(pages);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int vfio_pin_contiguous_pages_external(struct vfio_iommu *iommu,
> >> +				    struct vfio_dma *dma,
> >> +				    unsigned long *user_pfn,
> >> +				    int npage, unsigned long *phys_pfn,
> >> +				    bool do_accounting)
> >> +{
> >> +	int ret, i, j, lock_acct = 0;
> >> +	unsigned long remote_vaddr;
> >> +	dma_addr_t iova;
> >> +	struct mm_struct *mm;
> >> +	struct vfio_pfn *vpfn;
> >> +
> >> +	mm = get_task_mm(dma->task);
> >> +	if (!mm)
> >> +		return -ENODEV;
> >> +
> >> +	iova = user_pfn[0] << PAGE_SHIFT;
> >> +	remote_vaddr = dma->vaddr + iova - dma->iova;
> >> +	ret = contiguous_vaddr_get_pfn(mm, remote_vaddr, dma->prot,
> >> +					    npage, phys_pfn);
> >> +	mmput(mm);
> >> +	if (ret <= 0)
> >> +		return ret;
> >> +
> >> +	npage = ret;
> >> +	for (i = 0; i < npage; i++) {
> >> +		iova = user_pfn[i] << PAGE_SHIFT;
> >> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> >> +		if (ret)
> >> +			goto unwind;
> >> +
> >> +		if (!is_invalid_reserved_pfn(phys_pfn[i]))
> >> +			lock_acct++;
> >> +
> >> +		if (iommu->dirty_page_tracking) {
> >> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> >> +
> >> +			/*
> >> +			 * Bitmap populated with the smallest supported page
> >> +			 * size
> >> +			 */
> >> +			bitmap_set(dma->bitmap,
> >> +				   (iova - dma->iova) >> pgshift, 1);
> >> +		}  
> > 
> > It doesn't make sense that we wouldn't also optimize this to simply set
> > npage bits.  There's also no unwind for this.
> >   
> Thank you for your correction, I should set npage to simplify.
> There is no unwind process on the current master branch.  Is this a bug?

It's a bit tricky to know when it's ok to clear a bit in the dirty
bitmap, it's much, much worse to accidentally clear a bit to
incorrectly report a page a clean than it is to fail to unwind and
leave pages marked as dirty.  Given that this is an error path, we
might be willing to incorrectly leave pages marked dirty rather than
risk the alternative.

> >> +	}
> >> +
> >> +	if (do_accounting) {
> >> +		ret = vfio_lock_acct(dma, lock_acct, true);
> >> +		if (ret) {
> >> +			if (ret == -ENOMEM)
> >> +				pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
> >> +					__func__, dma->task->comm, task_pid_nr(dma->task),
> >> +					task_rlimit(dma->task, RLIMIT_MEMLOCK));
> >> +			goto unwind;
> >> +		}
> >> +	}  
> > 
> > This algorithm allows pinning many more pages in advance of testing
> > whether we've exceeded the task locked memory limit than the per page
> > approach.
> >   
> More than 1~VFIO_PIN_PAGES_MAX_ENTRIES. But after failure, all pinned
> pages will be released. Is there a big impact here?
> >   
> >> +
> >> +	return i;
> >> +unwind:
> >> +	for (j = 0; j < npage; j++) {
> >> +		put_pfn(phys_pfn[j], dma->prot);
> >> +		phys_pfn[j] = 0;
> >> +	}
> >> +
> >> +	for (j = 0; j < i; j++) {
> >> +		iova = user_pfn[j] << PAGE_SHIFT;
> >> +		vpfn = vfio_find_vpfn(dma, iova);
> >> +		if (vpfn)  
> > 
> > Seems like not finding a vpfn would be an error.
> >   
> I think the above process can ensure that vpfn is not NULL
> and delete this judgment.
> >> +			vfio_remove_from_pfn_list(dma, vpfn);  
> > 
> > It seems poor form not to use vfio_iova_put_vfio_pfn() here even if we
> > know we hold the only reference.
> >   
> The logic here can be reduced to a loop.
> When j < i, call vfio_iova_put_vfio_pfn.
> >   
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int vfio_iommu_type1_pin_contiguous_pages(struct vfio_iommu *iommu,
> >> +					    struct vfio_dma *dma,
> >> +					    unsigned long *user_pfn,
> >> +					    int npage, unsigned long *phys_pfn,
> >> +					    bool do_accounting)
> >> +{
> >> +	int ret, i, j;
> >> +	unsigned long remote_vaddr;
> >> +	dma_addr_t iova;
> >> +
> >> +	ret = vfio_pin_contiguous_pages_external(iommu, dma, user_pfn, npage,
> >> +				phys_pfn, do_accounting);
> >> +	if (ret == npage)
> >> +		return ret;
> >> +
> >> +	if (ret < 0)
> >> +		ret = 0;  
> > 
> > 
> > I'm lost, why do we need the single page iteration below??
> >   
> Since there is no retry in contiguous_vaddr_get_pfn, oncean error occurs,
> the remaining pages will be processed in a single page.
> Is it better to increase retry in contiguous_vaddr_get_pfn?

Do we expect it to work if we call it again?  Do we expect the below
single page iteration to work if the npage pinning failed?  Why?


> >> +	for (i = ret; i < npage; i++) {
> >> +		iova = user_pfn[i] << PAGE_SHIFT;
> >> +		remote_vaddr = dma->vaddr + iova - dma->iova;
> >> +
> >> +		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
> >> +			    do_accounting);
> >> +		if (ret)
> >> +			goto pin_unwind;
> >> +
> >> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> >> +		if (ret) {
> >> +			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
> >> +				vfio_lock_acct(dma, -1, true);
> >> +			goto pin_unwind;
> >> +		}
> >> +
> >> +		if (iommu->dirty_page_tracking) {
> >> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> >> +
> >> +			/*
> >> +			 * Bitmap populated with the smallest supported page
> >> +			 * size
> >> +			 */
> >> +			bitmap_set(dma->bitmap,
> >> +					   (iova - dma->iova) >> pgshift, 1);
> >> +		}
> >> +	}
> >> +
> >> +	return i;
> >> +
> >> +pin_unwind:
> >> +	phys_pfn[i] = 0;
> >> +	for (j = 0; j < i; j++) {
> >> +		dma_addr_t iova;
> >> +
> >> +		iova = user_pfn[j] << PAGE_SHIFT;
> >> +		vfio_unpin_page_external(dma, iova, do_accounting);
> >> +		phys_pfn[j] = 0;
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int vfio_iommu_type1_get_contiguous_pages_length(struct vfio_iommu *iommu,
> >> +				    unsigned long *user_pfn, int npage, int prot)
> >> +{
> >> +	struct vfio_dma *dma_base;
> >> +	int i;
> >> +	dma_addr_t iova;
> >> +	struct vfio_pfn *vpfn;
> >> +
> >> +	if (npage <= 1)
> >> +		return npage;
> >> +
> >> +	iova = user_pfn[0] << PAGE_SHIFT;
> >> +	dma_base = vfio_find_dma(iommu, iova, PAGE_SIZE);
> >> +	if (!dma_base)
> >> +		return -EINVAL;
> >> +
> >> +	if ((dma_base->prot & prot) != prot)
> >> +		return -EPERM;
> >> +
> >> +	for (i = 1; i < npage; i++) {
> >> +		iova = user_pfn[i] << PAGE_SHIFT;
> >> +
> >> +		if (iova >= dma_base->iova + dma_base->size ||
> >> +				iova + PAGE_SIZE <= dma_base->iova)
> >> +			break;
> >> +
> >> +		vpfn = vfio_iova_get_vfio_pfn(dma_base, iova);
> >> +		if (vpfn) {
> >> +			vfio_iova_put_vfio_pfn(dma_base, vpfn);  
> > 
> > Why not just use vfio_find_vpfn() rather than get+put?
> >   
> Thank you for your correction, I should use vfio_find_vpfn here.
> >> +			break;
> >> +		}
> >> +
> >> +		if (user_pfn[i] != user_pfn[0] + i)  
> > 
> > Shouldn't this be the first test?
> >   
> Thank you for your correction, the least costly judgment should be
> the first test.
> >> +			break;
> >> +	}
> >> +	return i;
> >> +}
> >> +
> >>  static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>  				      struct iommu_group *iommu_group,
> >>  				      unsigned long *user_pfn,
> >> @@ -637,9 +837,9 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>  	struct vfio_iommu *iommu = iommu_data;
> >>  	struct vfio_group *group;
> >>  	int i, j, ret;
> >> -	unsigned long remote_vaddr;
> >>  	struct vfio_dma *dma;
> >>  	bool do_accounting;
> >> +	int contiguous_npage;
> >>
> >>  	if (!iommu || !user_pfn || !phys_pfn)
> >>  		return -EINVAL;
> >> @@ -663,7 +863,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>  	 */
> >>  	do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
> >>
> >> -	for (i = 0; i < npage; i++) {
> >> +	for (i = 0; i < npage; i += contiguous_npage) {
> >>  		dma_addr_t iova;
> >>  		struct vfio_pfn *vpfn;
> >>
> >> @@ -682,31 +882,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>  		vpfn = vfio_iova_get_vfio_pfn(dma, iova);
> >>  		if (vpfn) {
> >>  			phys_pfn[i] = vpfn->pfn;
> >> -			continue;
> >> -		}
> >> -
> >> -		remote_vaddr = dma->vaddr + (iova - dma->iova);
> >> -		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
> >> -					     do_accounting);
> >> -		if (ret)
> >> -			goto pin_unwind;
> >> -
> >> -		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> >> -		if (ret) {
> >> -			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
> >> -				vfio_lock_acct(dma, -1, true);
> >> -			goto pin_unwind;
> >> -		}
> >> -
> >> -		if (iommu->dirty_page_tracking) {
> >> -			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> >> -
> >> -			/*
> >> -			 * Bitmap populated with the smallest supported page
> >> -			 * size
> >> -			 */
> >> -			bitmap_set(dma->bitmap,
> >> -				   (iova - dma->iova) >> pgshift, 1);
> >> +			contiguous_npage = 1;
> >> +		} else {
> >> +			ret = vfio_iommu_type1_get_contiguous_pages_length(iommu,
> >> +					&user_pfn[i], npage - i, prot);  
> > 
> > 
> > It doesn't make a lot of sense to me that this isn't more integrated
> > into the base function.  For example, we're passing &user_pfn[i] for
> > which we've already converted to an iova, found the vfio_dma associated
> > to that iova, and checked the protection.  This callout does all of
> > that again on the same.
> >   
> Thanks for your correction, I will delete the redundant check in
> vfio_iommu_type1_get_contiguous_pages_length and simplify the function to
> static int vfio_get_contiguous_pages_length (struct vfio_dma * dma,
> unsigned long * user_pfn, int npage)
> >> +			if (ret < 0)
> >> +				goto pin_unwind;
> >> +
> >> +			ret = vfio_iommu_type1_pin_contiguous_pages(iommu,
> >> +					dma, &user_pfn[i], ret, &phys_pfn[i], do_accounting);
> >> +			if (ret < 0)
> >> +				goto pin_unwind;
> >> +			contiguous_npage = ret;
> >>  		}
> >>  	}
> >>  	ret = i;  
> > 
> > 
> > This all seems _way_ more complicated than it needs to be, there are
> > too many different ways to flow through this code and claims of a
> > performance improvement are not substantiated with evidence.  The type1
> > code is already too fragile.  Please simplify and justify.  Thanks,
> > 
> > Alex
> > 
> > .
> >   
> The main issue is the balance between performance and complexity.
> The test data has been given above, please tell me your opinion.

I think the implementation here is overly complicated, there are too
many code paths and it's not clear what real world improvement to
expect.  The test data only shows us the theoretical best case
improvement of optimizing for a specific use case, without indicating
how prevalent or frequent that use case occurs in operation of an
actual device.  I suspect that it's possible to make the optimization
you're trying to achieve without this degree of complexity.  Thanks,

Alex

Re: [PATCH] vfio iommu type1: Improve vfio_iommu_type1_pin_pages performance

[xuxiaoyang (C)]

On 2020/11/17 0:33, Alex Williamson wrote:
> On Mon, 16 Nov 2020 21:47:33 +0800
> "xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:
> 
>> On 2020/11/14 0:44, Alex Williamson wrote:
>>> On Tue, 10 Nov 2020 21:42:33 +0800
>>> "xuxiaoyang (C)" <xuxiaoyang2@huawei.com> wrote:
>>>   
>>>> vfio_iommu_type1_pin_pages is very inefficient because
>>>> it is processed page by page when calling vfio_pin_page_external.
>>>> Added contiguous_vaddr_get_pfn to process continuous pages
>>>> to reduce the number of loops, thereby improving performance.
>>>>
>>>> Signed-off-by: Xiaoyang Xu <xuxiaoyang2@huawei.com>
>>>> ---
>>>>  drivers/vfio/vfio_iommu_type1.c | 241 ++++++++++++++++++++++++++++----
>>>>  1 file changed, 214 insertions(+), 27 deletions(-)  
>>>
>>> Sorry for my previous misunderstanding of the logic here.  Still, this
>>> adds a lot of complexity, can you quantify the performance improvement
>>> you're seeing?  Would it instead be more worthwhile to support an
>>> interface that pins based on an iova and range?  Further comments below.
>>>   
>> Thank you for your reply.  I have a set of performance test data for reference.
>> The host kernel version of my test environment is 4.19.36, and the test cases
>> are for pin one page and 512 pages.  When pin 512pages, the input is a
>> continuous iova address.  At the same time increase the measurement factor of
>> whether the mem backend uses large pages.  The following is the average of
>> multiple tests.
>>
>> The patch was not applied
>>                     1 page           512 pages
>> no huge pages：     1638ns           223651ns
>> THP：               1668ns           222330ns
>> HugeTLB：           1526ns           208151ns
>>
>> The patch was applied
>>                     1 page           512 pages
>> no huge pages       1735ns           167286ns
>> THP：               1934ns           126900ns
>> HugeTLB：           1713ns           102188ns
>>
>> The performance will be reduced when the page is fixed with a single pin,
>> while the page time of continuous pins can be reduced to half of the original.
>> If you have other suggestions for testing methods, please let me know.
> 
> These are artificial test results, which in fact show a performance
> decrease for the single page use cases.  What do we see in typical real
> world scenarios?  Do those real world scenarios tend more towards large
> arrays of contiguous IOVAs or single pages, or large arrays of
> discontiguous IOVAs?  What's the resulting change in device
> performance?  Are pages pinned at a high enough frequency that pinning
> latency results in a measurable benefit at the device or to the
> overhead on the host?
> 
Thank you for your reply.  It is difficult for me to construct these
use cases.  When the device is performing DMA, frequent calls to
pin/unpin pages are a big overhead, so we will pin a large part of
the memory to create a memory pool at the beginning.  Therefore, we
pay more attention to the performance of pin continuous pages.
Single page is processed in contiguous_vaddr_get_pfn, which will
reduce performance. We can keep the previous code processing path
in vfio_pin_page_external.  As long as vfio_get_contiguous_pages_length
can return fast enough, the performance loss of single page processing
will be minimized.
>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>>> index 67e827638995..935f80807527 100644
>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>> @@ -628,6 +628,206 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
>>>>  	return unlocked;
>>>>  }
>>>>
>>>> +static int contiguous_vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
>>>> +				    int prot, long npage, unsigned long *phys_pfn)
>>>> +{
>>>> +	struct page **pages = NULL;
>>>> +	unsigned int flags = 0;
>>>> +	int i, ret;
>>>> +
>>>> +	pages = kvmalloc_array(npage, sizeof(struct page *), GFP_KERNEL);
>>>> +	if (!pages)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	if (prot & IOMMU_WRITE)
>>>> +		flags |= FOLL_WRITE;
>>>> +
>>>> +	mmap_read_lock(mm);
>>>> +	ret = pin_user_pages_remote(mm, vaddr, npage, flags | FOLL_LONGTERM,
>>>> +				    pages, NULL, NULL);  
>>>
>>> This is essentially the root of the performance improvement claim,
>>> right?  ie. we're pinning a range of pages rather than individually.
>>> Unfortunately it means we also add a dynamic memory allocation even
>>> when npage=1.
>>>   
>> Yes, when npage = 1, I should keep the previous scheme of the scene
>> and use local variables to save time in allocated memory
>>>   
>>>> +	mmap_read_unlock(mm);
>>>> +
>>>> +	for (i = 0; i < ret; i++)
>>>> +		*(phys_pfn + i) = page_to_pfn(pages[i]);
>>>> +
>>>> +	kvfree(pages);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int vfio_pin_contiguous_pages_external(struct vfio_iommu *iommu,
>>>> +				    struct vfio_dma *dma,
>>>> +				    unsigned long *user_pfn,
>>>> +				    int npage, unsigned long *phys_pfn,
>>>> +				    bool do_accounting)
>>>> +{
>>>> +	int ret, i, j, lock_acct = 0;
>>>> +	unsigned long remote_vaddr;
>>>> +	dma_addr_t iova;
>>>> +	struct mm_struct *mm;
>>>> +	struct vfio_pfn *vpfn;
>>>> +
>>>> +	mm = get_task_mm(dma->task);
>>>> +	if (!mm)
>>>> +		return -ENODEV;
>>>> +
>>>> +	iova = user_pfn[0] << PAGE_SHIFT;
>>>> +	remote_vaddr = dma->vaddr + iova - dma->iova;
>>>> +	ret = contiguous_vaddr_get_pfn(mm, remote_vaddr, dma->prot,
>>>> +					    npage, phys_pfn);
>>>> +	mmput(mm);
>>>> +	if (ret <= 0)
>>>> +		return ret;
>>>> +
>>>> +	npage = ret;
>>>> +	for (i = 0; i < npage; i++) {
>>>> +		iova = user_pfn[i] << PAGE_SHIFT;
>>>> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
>>>> +		if (ret)
>>>> +			goto unwind;
>>>> +
>>>> +		if (!is_invalid_reserved_pfn(phys_pfn[i]))
>>>> +			lock_acct++;
>>>> +
>>>> +		if (iommu->dirty_page_tracking) {
>>>> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
>>>> +
>>>> +			/*
>>>> +			 * Bitmap populated with the smallest supported page
>>>> +			 * size
>>>> +			 */
>>>> +			bitmap_set(dma->bitmap,
>>>> +				   (iova - dma->iova) >> pgshift, 1);
>>>> +		}  
>>>
>>> It doesn't make sense that we wouldn't also optimize this to simply set
>>> npage bits.  There's also no unwind for this.
>>>   
>> Thank you for your correction, I should set npage to simplify.
>> There is no unwind process on the current master branch.  Is this a bug?
> 
> It's a bit tricky to know when it's ok to clear a bit in the dirty
> bitmap, it's much, much worse to accidentally clear a bit to
> incorrectly report a page a clean than it is to fail to unwind and
> leave pages marked as dirty.  Given that this is an error path, we
> might be willing to incorrectly leave pages marked dirty rather than
> risk the alternative.
> 
>>>> +	}
>>>> +
>>>> +	if (do_accounting) {
>>>> +		ret = vfio_lock_acct(dma, lock_acct, true);
>>>> +		if (ret) {
>>>> +			if (ret == -ENOMEM)
>>>> +				pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
>>>> +					__func__, dma->task->comm, task_pid_nr(dma->task),
>>>> +					task_rlimit(dma->task, RLIMIT_MEMLOCK));
>>>> +			goto unwind;
>>>> +		}
>>>> +	}  
>>>
>>> This algorithm allows pinning many more pages in advance of testing
>>> whether we've exceeded the task locked memory limit than the per page
>>> approach.
>>>   
>> More than 1~VFIO_PIN_PAGES_MAX_ENTRIES. But after failure, all pinned
>> pages will be released. Is there a big impact here?
>>>   
>>>> +
>>>> +	return i;
>>>> +unwind:
>>>> +	for (j = 0; j < npage; j++) {
>>>> +		put_pfn(phys_pfn[j], dma->prot);
>>>> +		phys_pfn[j] = 0;
>>>> +	}
>>>> +
>>>> +	for (j = 0; j < i; j++) {
>>>> +		iova = user_pfn[j] << PAGE_SHIFT;
>>>> +		vpfn = vfio_find_vpfn(dma, iova);
>>>> +		if (vpfn)  
>>>
>>> Seems like not finding a vpfn would be an error.
>>>   
>> I think the above process can ensure that vpfn is not NULL
>> and delete this judgment.
>>>> +			vfio_remove_from_pfn_list(dma, vpfn);  
>>>
>>> It seems poor form not to use vfio_iova_put_vfio_pfn() here even if we
>>> know we hold the only reference.
>>>   
>> The logic here can be reduced to a loop.
>> When j < i, call vfio_iova_put_vfio_pfn.
>>>   
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int vfio_iommu_type1_pin_contiguous_pages(struct vfio_iommu *iommu,
>>>> +					    struct vfio_dma *dma,
>>>> +					    unsigned long *user_pfn,
>>>> +					    int npage, unsigned long *phys_pfn,
>>>> +					    bool do_accounting)
>>>> +{
>>>> +	int ret, i, j;
>>>> +	unsigned long remote_vaddr;
>>>> +	dma_addr_t iova;
>>>> +
>>>> +	ret = vfio_pin_contiguous_pages_external(iommu, dma, user_pfn, npage,
>>>> +				phys_pfn, do_accounting);
>>>> +	if (ret == npage)
>>>> +		return ret;
>>>> +
>>>> +	if (ret < 0)
>>>> +		ret = 0;  
>>>
>>>
>>> I'm lost, why do we need the single page iteration below??
>>>   
>> Since there is no retry in contiguous_vaddr_get_pfn, oncean error occurs,
>> the remaining pages will be processed in a single page.
>> Is it better to increase retry in contiguous_vaddr_get_pfn?
> 
> Do we expect it to work if we call it again?  Do we expect the below
> single page iteration to work if the npage pinning failed?  Why?
> 
Vaddr_get_pfn contains error handling for the VM_PFNMAP type of
mapping area.  In this case, we hope he can continue to work.
As mentioned above, the scene of n=1 will be processed directly
using vfio_pin_page_external.
> 
>>>> +	for (i = ret; i < npage; i++) {
>>>> +		iova = user_pfn[i] << PAGE_SHIFT;
>>>> +		remote_vaddr = dma->vaddr + iova - dma->iova;
>>>> +
>>>> +		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
>>>> +			    do_accounting);
>>>> +		if (ret)
>>>> +			goto pin_unwind;
>>>> +
>>>> +		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
>>>> +		if (ret) {
>>>> +			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
>>>> +				vfio_lock_acct(dma, -1, true);
>>>> +			goto pin_unwind;
>>>> +		}
>>>> +
>>>> +		if (iommu->dirty_page_tracking) {
>>>> +			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
>>>> +
>>>> +			/*
>>>> +			 * Bitmap populated with the smallest supported page
>>>> +			 * size
>>>> +			 */
>>>> +			bitmap_set(dma->bitmap,
>>>> +					   (iova - dma->iova) >> pgshift, 1);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return i;
>>>> +
>>>> +pin_unwind:
>>>> +	phys_pfn[i] = 0;
>>>> +	for (j = 0; j < i; j++) {
>>>> +		dma_addr_t iova;
>>>> +
>>>> +		iova = user_pfn[j] << PAGE_SHIFT;
>>>> +		vfio_unpin_page_external(dma, iova, do_accounting);
>>>> +		phys_pfn[j] = 0;
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int vfio_iommu_type1_get_contiguous_pages_length(struct vfio_iommu *iommu,
>>>> +				    unsigned long *user_pfn, int npage, int prot)
>>>> +{
>>>> +	struct vfio_dma *dma_base;
>>>> +	int i;
>>>> +	dma_addr_t iova;
>>>> +	struct vfio_pfn *vpfn;
>>>> +
>>>> +	if (npage <= 1)
>>>> +		return npage;
>>>> +
>>>> +	iova = user_pfn[0] << PAGE_SHIFT;
>>>> +	dma_base = vfio_find_dma(iommu, iova, PAGE_SIZE);
>>>> +	if (!dma_base)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if ((dma_base->prot & prot) != prot)
>>>> +		return -EPERM;
>>>> +
>>>> +	for (i = 1; i < npage; i++) {
>>>> +		iova = user_pfn[i] << PAGE_SHIFT;
>>>> +
>>>> +		if (iova >= dma_base->iova + dma_base->size ||
>>>> +				iova + PAGE_SIZE <= dma_base->iova)
>>>> +			break;
>>>> +
>>>> +		vpfn = vfio_iova_get_vfio_pfn(dma_base, iova);
>>>> +		if (vpfn) {
>>>> +			vfio_iova_put_vfio_pfn(dma_base, vpfn);  
>>>
>>> Why not just use vfio_find_vpfn() rather than get+put?
>>>   
>> Thank you for your correction, I should use vfio_find_vpfn here.
>>>> +			break;
>>>> +		}
>>>> +
>>>> +		if (user_pfn[i] != user_pfn[0] + i)  
>>>
>>> Shouldn't this be the first test?
>>>   
>> Thank you for your correction, the least costly judgment should be
>> the first test.
>>>> +			break;
>>>> +	}
>>>> +	return i;
>>>> +}
>>>> +
>>>>  static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>>>  				      struct iommu_group *iommu_group,
>>>>  				      unsigned long *user_pfn,
>>>> @@ -637,9 +837,9 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>>>  	struct vfio_iommu *iommu = iommu_data;
>>>>  	struct vfio_group *group;
>>>>  	int i, j, ret;
>>>> -	unsigned long remote_vaddr;
>>>>  	struct vfio_dma *dma;
>>>>  	bool do_accounting;
>>>> +	int contiguous_npage;
>>>>
>>>>  	if (!iommu || !user_pfn || !phys_pfn)
>>>>  		return -EINVAL;
>>>> @@ -663,7 +863,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>>>  	 */
>>>>  	do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
>>>>
>>>> -	for (i = 0; i < npage; i++) {
>>>> +	for (i = 0; i < npage; i += contiguous_npage) {
>>>>  		dma_addr_t iova;
>>>>  		struct vfio_pfn *vpfn;
>>>>
>>>> @@ -682,31 +882,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>>>  		vpfn = vfio_iova_get_vfio_pfn(dma, iova);
>>>>  		if (vpfn) {
>>>>  			phys_pfn[i] = vpfn->pfn;
>>>> -			continue;
>>>> -		}
>>>> -
>>>> -		remote_vaddr = dma->vaddr + (iova - dma->iova);
>>>> -		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
>>>> -					     do_accounting);
>>>> -		if (ret)
>>>> -			goto pin_unwind;
>>>> -
>>>> -		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
>>>> -		if (ret) {
>>>> -			if (put_pfn(phys_pfn[i], dma->prot) && do_accounting)
>>>> -				vfio_lock_acct(dma, -1, true);
>>>> -			goto pin_unwind;
>>>> -		}
>>>> -
>>>> -		if (iommu->dirty_page_tracking) {
>>>> -			unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
>>>> -
>>>> -			/*
>>>> -			 * Bitmap populated with the smallest supported page
>>>> -			 * size
>>>> -			 */
>>>> -			bitmap_set(dma->bitmap,
>>>> -				   (iova - dma->iova) >> pgshift, 1);
>>>> +			contiguous_npage = 1;
>>>> +		} else {
>>>> +			ret = vfio_iommu_type1_get_contiguous_pages_length(iommu,
>>>> +					&user_pfn[i], npage - i, prot);  
>>>
>>>
>>> It doesn't make a lot of sense to me that this isn't more integrated
>>> into the base function.  For example, we're passing &user_pfn[i] for
>>> which we've already converted to an iova, found the vfio_dma associated
>>> to that iova, and checked the protection.  This callout does all of
>>> that again on the same.
>>>   
>> Thanks for your correction, I will delete the redundant check in
>> vfio_iommu_type1_get_contiguous_pages_length and simplify the function to
>> static int vfio_get_contiguous_pages_length (struct vfio_dma * dma,
>> unsigned long * user_pfn, int npage)
>>>> +			if (ret < 0)
>>>> +				goto pin_unwind;
>>>> +
>>>> +			ret = vfio_iommu_type1_pin_contiguous_pages(iommu,
>>>> +					dma, &user_pfn[i], ret, &phys_pfn[i], do_accounting);
>>>> +			if (ret < 0)
>>>> +				goto pin_unwind;
>>>> +			contiguous_npage = ret;
>>>>  		}
>>>>  	}
>>>>  	ret = i;  
>>>
>>>
>>> This all seems _way_ more complicated than it needs to be, there are
>>> too many different ways to flow through this code and claims of a
>>> performance improvement are not substantiated with evidence.  The type1
>>> code is already too fragile.  Please simplify and justify.  Thanks,
>>>
>>> Alex
>>>
>>> .
>>>   
>> The main issue is the balance between performance and complexity.
>> The test data has been given above, please tell me your opinion.
> 
> I think the implementation here is overly complicated, there are too
> many code paths and it's not clear what real world improvement to
> expect.  The test data only shows us the theoretical best case
> improvement of optimizing for a specific use case, without indicating
> how prevalent or frequent that use case occurs in operation of an
> actual device.  I suspect that it's possible to make the optimization
> you're trying to achieve without this degree of complexity.  Thanks,
> 
> Alex
> 
> .
> 
More other real-world improvements may require feedback from others.
I will post another patch to fix the previous bug and improve the
performance of the pin page.  Maybe others will make some improvements
on this basis.

Regards,
Xu