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@xxxxxxxxxx> wrote:
> 
>> On 2020/11/14 0:44, Alex Williamson wrote:
>>> On Tue, 10 Nov 2020 21:42:33 +0800
>>> "xuxiaoyang (C)" <xuxiaoyang2@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> ---
>>>>  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