Re: [PATCH v7 3/4] vfio iommu: Add support for mediated devices

[Jike Song <jike.song@intel.com>]

On 09/30/2016 07:44 PM, Kirti Wankhede wrote:
> On 9/30/2016 8:40 AM, Jike Song wrote:
>> On 09/30/2016 10:58 AM, Jike Song wrote:
>>> On 09/29/2016 11:06 PM, Kirti Wankhede wrote:
>>>>
>>>>
>>>> On 9/29/2016 7:47 AM, Jike Song wrote:
>>>>> +Guangrong
>>>>>
>>>>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
>>>>
>>>> ...
>>>>
>>>>>> +static long vfio_iommu_type1_pin_pages(void *iommu_data,
>>>>>> +				       unsigned long *user_pfn,
>>>>>> +				       long npage, int prot,
>>>>>> +				       unsigned long *phys_pfn)
>>>>>> +{
>>>>>> +	struct vfio_iommu *iommu = iommu_data;
>>>>>> +	struct vfio_domain *domain;
>>>>>> +	int i, j, ret;
>>>>>> +	long retpage;
>>>>>> +	unsigned long remote_vaddr;
>>>>>> +	unsigned long *pfn = phys_pfn;
>>>>>> +	struct vfio_dma *dma;
>>>>>> +	bool do_accounting = false;
>>>>>> +
>>>>>> +	if (!iommu || !user_pfn || !phys_pfn)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	mutex_lock(&iommu->lock);
>>>>>> +
>>>>>> +	if (!iommu->local_domain) {
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto pin_done;
>>>>>> +	}
>>>>>> +
>>>>>> +	domain = iommu->local_domain;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If iommu capable domain exist in the container then all pages are
>>>>>> +	 * already pinned and accounted. Accouting should be done if there is no
>>>>>> +	 * iommu capable domain in the container.
>>>>>> +	 */
>>>>>> +	do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
>>>>>> +
>>>>>> +	for (i = 0; i < npage; i++) {
>>>>>> +		struct vfio_pfn *p;
>>>>>> +		dma_addr_t iova;
>>>>>> +
>>>>>> +		iova = user_pfn[i] << PAGE_SHIFT;
>>>>>> +
>>>>>> +		dma = vfio_find_dma(iommu, iova, 0);
>>>>>> +		if (!dma) {
>>>>>> +			ret = -EINVAL;
>>>>>> +			goto pin_unwind;
>>>>>> +		}
>>>>>> +
>>>>>> +		remote_vaddr = dma->vaddr + iova - dma->iova;
>>>>>> +
>>>>>> +		retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
>>>>>> +						 &pfn[i], do_accounting);
>>>>>
>>>>> Hi Kirti,
>>>>>
>>>>> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
>>>>> whether the vaddr already pinned or not. That probably means, if the caller 
>>>>> calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.
>>>>>
>>>>> GUP always increases the page refcnt.
>>>>>
>>>>> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
>>>>> so you can always try to find the PFN for a given iova, and pin it only if
>>>>> not found.
>>>>>
>>>>
>>>> I didn't get how there would be a memory leak.
>>>>
>>>> Right, GUP increases refcnt, so if vfio_pin_pages() is called for
>>>> multiple types for same GPA, refcnt would be incremented. In
>>>> vfio_iommu_type1_pin_pages() pinned pages list is maintained with
>>>> ref_count. If pfn is already in list, ref_count is incremented and same
>>>> is used while unpining pages.
>>>>
>>>
>>> Let's have a close look at vfio_unpin_pfn:
>>>
>>> 	static int vfio_unpin_pfn(struct vfio_domain *domain,
>>> 				  struct vfio_pfn *vpfn, bool do_accounting)
>>> 	{
>>> 		__vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
>>> 					    do_accounting);
>>>
>>> 		if (atomic_dec_and_test(&vpfn->ref_count))
>>> 			vfio_remove_from_pfn_list(domain, vpfn);
>>>
>>> 		return 1;
>>> 	}
>>>
>>> Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
>>> vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, here
>>> you only set it back to (N-1).
>>>
> 
> User of vfio_pin_pages() should call vfio_unpin_pages() also,  so here
> we unpin it once. If vfio_pin_pages() is called twice for same page, we
> should get vfio_unpin_pages() twice for same page.
>

If this is the deliberate design, why do you need a 'ref_count'? You can
simply drop the 'ref_count' and blame the caller for pinning/unpinning
different times.

> If users of these APIs don't follow this, then
> vfio_release_domain() -> vfio_local_unpin_all() takes care of unpin,
> decrement ref_count and delete node on (ref_count == 0) for all
> remaining pfn.
>

Here you did pay attention to the "caller doesn't follow this" situation.
However, dealing with 'ref_count' in vfio-iommu is not enough: memory
leaked.

>>
>> What's more, since all pinned {iova, pfni} already saved, it's better to
>> consult it before calling GUP, which will get_page() unconditionally.
>
> pfn is required to unpin page, so we have pfn as key for rbtree.
> vfio_pin_pages() is called with user_pfn or iova, which can't be used to
> search in rbtree with iova in optimized way. Raw way would be to goto
> each node of rbtree and check iova which would hamper the performance in
> if this is called in performance critical path.
> So here optimized way is to first pin it, get pfn and check if already
> exist in rbtree. If it exist increment ref_count else add it to the rbtree.
> 

Of course pfn is required to unpin page, I 100% agree. But that doesn't 
change the argue: using iova instead for key, you can still store pfn
along with it.

By the way, calling GUP unconditionally hurts more than searching rbtree.


--
Thanks,
Jike