[PATCH] drm/amdgpu: use new HMM APIs and helpers

[PATCH] drm/amdgpu: use new HMM APIs and helpers

[Yang, Philip]

HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
path. The old hmm APIs are deprecated and will be removed in future.

Below are changes in driver:

1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
supports range with multiple vmas, remove the multiple vmas handle path
and data structure.
2. Change hmm_vma_range_done to hmm_range_unregister.
3. Use default flags to avoid pre-fill pfn arrays.
4. Use new hmm_device_ helpers.

Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c  |   1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 140 +++++++++---------------
 2 files changed, 53 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
index 41ccee49a224..e0bb47ce570b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
@@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
 		range->flags = hmm_range_flags;
 		range->values = hmm_range_values;
 		range->pfn_shift = PAGE_SHIFT;
-		range->pfns = NULL;
 		INIT_LIST_HEAD(&range->list);
 	}
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 7138dc1dd1f4..25a9fcb409c6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
 	struct task_struct	*usertask;
 	uint32_t		userflags;
 #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
-	struct hmm_range	*ranges;
-	int			nr_ranges;
+	struct hmm_range	*range;
 #endif
 };
 
@@ -725,57 +724,33 @@ struct amdgpu_ttm_tt {
  */
 #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
 
-/* Support Userptr pages cross max 16 vmas */
-#define MAX_NR_VMAS	(16)
-
 int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
 {
 	struct amdgpu_ttm_tt *gtt = (void *)ttm;
 	struct mm_struct *mm = gtt->usertask->mm;
 	unsigned long start = gtt->userptr;
-	unsigned long end = start + ttm->num_pages * PAGE_SIZE;
-	struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
-	struct hmm_range *ranges;
-	unsigned long nr_pages, i;
-	uint64_t *pfns, f;
+	struct vm_area_struct *vma;
+	struct hmm_range *range;
+	unsigned long i;
+	uint64_t *pfns;
 	int r = 0;
 
 	if (!mm) /* Happens during process shutdown */
 		return -ESRCH;
 
-	down_read(&mm->mmap_sem);
-
-	/* user pages may cross multiple VMAs */
-	gtt->nr_ranges = 0;
-	do {
-		unsigned long vm_start;
-
-		if (gtt->nr_ranges >= MAX_NR_VMAS) {
-			DRM_ERROR("Too many VMAs in userptr range\n");
-			r = -EFAULT;
-			goto out;
-		}
-
-		vm_start = vma ? vma->vm_end : start;
-		vma = find_vma(mm, vm_start);
-		if (unlikely(!vma || vm_start < vma->vm_start)) {
-			r = -EFAULT;
-			goto out;
-		}
-		vmas[gtt->nr_ranges++] = vma;
-	} while (end > vma->vm_end);
-
-	DRM_DEBUG_DRIVER("0x%lx nr_ranges %d pages 0x%lx\n",
-		start, gtt->nr_ranges, ttm->num_pages);
-
+	vma = find_vma(mm, start);
+	if (unlikely(!vma || start < vma->vm_start)) {
+		r = -EFAULT;
+		goto out;
+	}
 	if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
-		vmas[0]->vm_file)) {
+		vma->vm_file)) {
 		r = -EPERM;
 		goto out;
 	}
 
-	ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
-	if (unlikely(!ranges)) {
+	range = kvmalloc(sizeof(*range), GFP_KERNEL | __GFP_ZERO);
+	if (unlikely(!range)) {
 		r = -ENOMEM;
 		goto out;
 	}
@@ -786,61 +761,52 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
 		goto out_free_ranges;
 	}
 
-	for (i = 0; i < gtt->nr_ranges; i++)
-		amdgpu_hmm_init_range(&ranges[i]);
+	amdgpu_hmm_init_range(range);
+	range->default_flags = range->flags[HMM_PFN_VALID];
+	range->default_flags |= amdgpu_ttm_tt_is_readonly(ttm) ?
+				0 : range->flags[HMM_PFN_WRITE];
+	range->pfn_flags_mask = 0;
+	range->pfns = pfns;
+	hmm_range_register(range, mm, start,
+			   start + ttm->num_pages * PAGE_SIZE, PAGE_SHIFT);
 
-	f = ranges[0].flags[HMM_PFN_VALID];
-	f |= amdgpu_ttm_tt_is_readonly(ttm) ?
-				0 : ranges[0].flags[HMM_PFN_WRITE];
-	memset64(pfns, f, ttm->num_pages);
-
-	for (nr_pages = 0, i = 0; i < gtt->nr_ranges; i++) {
-		ranges[i].vma = vmas[i];
-		ranges[i].start = max(start, vmas[i]->vm_start);
-		ranges[i].end = min(end, vmas[i]->vm_end);
-		ranges[i].pfns = pfns + nr_pages;
-		nr_pages += (ranges[i].end - ranges[i].start) / PAGE_SIZE;
+	/*
+	 * Just wait for range to be valid, safe to ignore return value as we
+	 * will use the return value of hmm_range_fault() below under the
+	 * mmap_sem to ascertain the validity of the range.
+	*/
+	hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT);
 
-		r = hmm_vma_fault(&ranges[i], true);
-		if (unlikely(r))
-			break;
-	}
-	if (unlikely(r)) {
-		while (i--)
-			hmm_vma_range_done(&ranges[i]);
+	down_read(&mm->mmap_sem);
 
-		goto out_free_pfns;
-	}
+	r = hmm_range_fault(range, true);
 
 	up_read(&mm->mmap_sem);
 
+	if (unlikely(r < 0))
+		goto out_free_pfns;
+
 	for (i = 0; i < ttm->num_pages; i++) {
-		pages[i] = hmm_pfn_to_page(&ranges[0], pfns[i]);
-		if (!pages[i]) {
+		pages[i] = hmm_device_entry_to_page(range, pfns[i]);
+		if (unlikely(!pages[i])) {
 			pr_err("Page fault failed for pfn[%lu] = 0x%llx\n",
 			       i, pfns[i]);
-			goto out_invalid_pfn;
+			r = -ENOMEM;
+
+			goto out_free_pfns;
 		}
 	}
-	gtt->ranges = ranges;
+	gtt->range = range;
 
 	return 0;
 
 out_free_pfns:
+	hmm_range_unregister(range);
 	kvfree(pfns);
 out_free_ranges:
-	kvfree(ranges);
+	kvfree(range);
 out:
-	up_read(&mm->mmap_sem);
-
 	return r;
-
-out_invalid_pfn:
-	for (i = 0; i < gtt->nr_ranges; i++)
-		hmm_vma_range_done(&ranges[i]);
-	kvfree(pfns);
-	kvfree(ranges);
-	return -ENOMEM;
 }
 
 /**
@@ -853,23 +819,23 @@ bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm)
 {
 	struct amdgpu_ttm_tt *gtt = (void *)ttm;
 	bool r = false;
-	int i;
 
 	if (!gtt || !gtt->userptr)
 		return false;
 
-	DRM_DEBUG_DRIVER("user_pages_done 0x%llx nr_ranges %d pages 0x%lx\n",
-		gtt->userptr, gtt->nr_ranges, ttm->num_pages);
+	DRM_DEBUG_DRIVER("user_pages_done 0x%llx pages 0x%lx\n",
+		gtt->userptr, ttm->num_pages);
 
-	WARN_ONCE(!gtt->ranges || !gtt->ranges[0].pfns,
+	WARN_ONCE(!gtt->range || !gtt->range->pfns,
 		"No user pages to check\n");
 
-	if (gtt->ranges) {
-		for (i = 0; i < gtt->nr_ranges; i++)
-			r |= hmm_vma_range_done(&gtt->ranges[i]);
-		kvfree(gtt->ranges[0].pfns);
-		kvfree(gtt->ranges);
-		gtt->ranges = NULL;
+	if (gtt->range) {
+		r = hmm_range_valid(gtt->range);
+		hmm_range_unregister(gtt->range);
+
+		kvfree(gtt->range->pfns);
+		kvfree(gtt->range);
+		gtt->range = NULL;
 	}
 
 	return r;
@@ -953,9 +919,9 @@ static void amdgpu_ttm_tt_unpin_userptr(struct ttm_tt *ttm)
 	sg_free_table(ttm->sg);
 
 #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
-	if (gtt->ranges &&
-	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
-					     gtt->ranges[0].pfns[0]))
+	if (gtt->range &&
+	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
+						      gtt->range->pfns[0]))
 		WARN_ONCE(1, "Missing get_user_page_done\n");
 #endif
 }
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Kuehling, Felix]

This is a nice simplification. See a few comments inline.

On 2019-05-30 10:41 a.m., Yang, Philip wrote:
> HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
> path. The old hmm APIs are deprecated and will be removed in future.
>
> Below are changes in driver:
>
> 1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
> supports range with multiple vmas, remove the multiple vmas handle path
> and data structure.
> 2. Change hmm_vma_range_done to hmm_range_unregister.
> 3. Use default flags to avoid pre-fill pfn arrays.
> 4. Use new hmm_device_ helpers.
>
> Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c  |   1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 140 +++++++++---------------
>   2 files changed, 53 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> index 41ccee49a224..e0bb47ce570b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> @@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
>   		range->flags = hmm_range_flags;
>   		range->values = hmm_range_values;
>   		range->pfn_shift = PAGE_SHIFT;
> -		range->pfns = NULL;
>   		INIT_LIST_HEAD(&range->list);
>   	}
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 7138dc1dd1f4..25a9fcb409c6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
>   	struct task_struct	*usertask;
>   	uint32_t		userflags;
>   #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
> -	struct hmm_range	*ranges;
> -	int			nr_ranges;
> +	struct hmm_range	*range;
>   #endif
>   };
>   
> @@ -725,57 +724,33 @@ struct amdgpu_ttm_tt {
>    */
>   #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>   
> -/* Support Userptr pages cross max 16 vmas */
> -#define MAX_NR_VMAS	(16)
> -
>   int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>   {
>   	struct amdgpu_ttm_tt *gtt = (void *)ttm;
>   	struct mm_struct *mm = gtt->usertask->mm;
>   	unsigned long start = gtt->userptr;
> -	unsigned long end = start + ttm->num_pages * PAGE_SIZE;
> -	struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
> -	struct hmm_range *ranges;
> -	unsigned long nr_pages, i;
> -	uint64_t *pfns, f;
> +	struct vm_area_struct *vma;
> +	struct hmm_range *range;
> +	unsigned long i;
> +	uint64_t *pfns;
>   	int r = 0;
>   
>   	if (!mm) /* Happens during process shutdown */
>   		return -ESRCH;
>   
> -	down_read(&mm->mmap_sem);
> -
> -	/* user pages may cross multiple VMAs */
> -	gtt->nr_ranges = 0;
> -	do {
> -		unsigned long vm_start;
> -
> -		if (gtt->nr_ranges >= MAX_NR_VMAS) {
> -			DRM_ERROR("Too many VMAs in userptr range\n");
> -			r = -EFAULT;
> -			goto out;
> -		}
> -
> -		vm_start = vma ? vma->vm_end : start;
> -		vma = find_vma(mm, vm_start);
> -		if (unlikely(!vma || vm_start < vma->vm_start)) {
> -			r = -EFAULT;
> -			goto out;
> -		}
> -		vmas[gtt->nr_ranges++] = vma;
> -	} while (end > vma->vm_end);
> -
> -	DRM_DEBUG_DRIVER("0x%lx nr_ranges %d pages 0x%lx\n",
> -		start, gtt->nr_ranges, ttm->num_pages);
> -
> +	vma = find_vma(mm, start);
> +	if (unlikely(!vma || start < vma->vm_start)) {
> +		r = -EFAULT;
> +		goto out;
> +	}
>   	if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
> -		vmas[0]->vm_file)) {
> +		vma->vm_file)) {
>   		r = -EPERM;
>   		goto out;
>   	}
>   
> -	ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
> -	if (unlikely(!ranges)) {
> +	range = kvmalloc(sizeof(*range), GFP_KERNEL | __GFP_ZERO);

A single range is pretty small. You can probably allocate that with 
kmalloc now (or kcalloc/kzalloc since you specified GFP_ZERO).


> +	if (unlikely(!range)) {
>   		r = -ENOMEM;
>   		goto out;
>   	}
> @@ -786,61 +761,52 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>   		goto out_free_ranges;
>   	}
>   
> -	for (i = 0; i < gtt->nr_ranges; i++)
> -		amdgpu_hmm_init_range(&ranges[i]);
> +	amdgpu_hmm_init_range(range);
> +	range->default_flags = range->flags[HMM_PFN_VALID];
> +	range->default_flags |= amdgpu_ttm_tt_is_readonly(ttm) ?
> +				0 : range->flags[HMM_PFN_WRITE];
> +	range->pfn_flags_mask = 0;
> +	range->pfns = pfns;
> +	hmm_range_register(range, mm, start,
> +			   start + ttm->num_pages * PAGE_SIZE, PAGE_SHIFT);
>   
> -	f = ranges[0].flags[HMM_PFN_VALID];
> -	f |= amdgpu_ttm_tt_is_readonly(ttm) ?
> -				0 : ranges[0].flags[HMM_PFN_WRITE];
> -	memset64(pfns, f, ttm->num_pages);
> -
> -	for (nr_pages = 0, i = 0; i < gtt->nr_ranges; i++) {
> -		ranges[i].vma = vmas[i];
> -		ranges[i].start = max(start, vmas[i]->vm_start);
> -		ranges[i].end = min(end, vmas[i]->vm_end);
> -		ranges[i].pfns = pfns + nr_pages;
> -		nr_pages += (ranges[i].end - ranges[i].start) / PAGE_SIZE;
> +	/*
> +	 * Just wait for range to be valid, safe to ignore return value as we
> +	 * will use the return value of hmm_range_fault() below under the
> +	 * mmap_sem to ascertain the validity of the range.
> +	*/

Documentation/vm/hmm.rst suggests a retry if hmm_range_fault (or 
snapshot) returns -EAGAIN. I think this would be the right place to put 
the again: label.


> +	hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT);
>   
> -		r = hmm_vma_fault(&ranges[i], true);
> -		if (unlikely(r))
> -			break;
> -	}
> -	if (unlikely(r)) {
> -		while (i--)
> -			hmm_vma_range_done(&ranges[i]);
> +	down_read(&mm->mmap_sem);
>   
> -		goto out_free_pfns;
> -	}
> +	r = hmm_range_fault(range, true);
>   
>   	up_read(&mm->mmap_sem);
>   
> +	if (unlikely(r < 0))
> +		goto out_free_pfns;

If r == -EAGAIN, you could goto again for a retry here as suggested by 
the HMM documentation.


> +
>   	for (i = 0; i < ttm->num_pages; i++) {
> -		pages[i] = hmm_pfn_to_page(&ranges[0], pfns[i]);
> -		if (!pages[i]) {
> +		pages[i] = hmm_device_entry_to_page(range, pfns[i]);
> +		if (unlikely(!pages[i])) {
>   			pr_err("Page fault failed for pfn[%lu] = 0x%llx\n",
>   			       i, pfns[i]);
> -			goto out_invalid_pfn;
> +			r = -ENOMEM;
> +
> +			goto out_free_pfns;
>   		}
>   	}
> -	gtt->ranges = ranges;
> +	gtt->range = range;
>   
>   	return 0;
>   
>   out_free_pfns:
> +	hmm_range_unregister(range);
>   	kvfree(pfns);
>   out_free_ranges:
> -	kvfree(ranges);
> +	kvfree(range);

If you use kmalloc/kcalloc/kzalloc above, change this to kfree.


>   out:
> -	up_read(&mm->mmap_sem);
> -
>   	return r;
> -
> -out_invalid_pfn:
> -	for (i = 0; i < gtt->nr_ranges; i++)
> -		hmm_vma_range_done(&ranges[i]);
> -	kvfree(pfns);
> -	kvfree(ranges);
> -	return -ENOMEM;
>   }
>   
>   /**
> @@ -853,23 +819,23 @@ bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm)
>   {
>   	struct amdgpu_ttm_tt *gtt = (void *)ttm;
>   	bool r = false;
> -	int i;
>   
>   	if (!gtt || !gtt->userptr)
>   		return false;
>   
> -	DRM_DEBUG_DRIVER("user_pages_done 0x%llx nr_ranges %d pages 0x%lx\n",
> -		gtt->userptr, gtt->nr_ranges, ttm->num_pages);
> +	DRM_DEBUG_DRIVER("user_pages_done 0x%llx pages 0x%lx\n",
> +		gtt->userptr, ttm->num_pages);
>   
> -	WARN_ONCE(!gtt->ranges || !gtt->ranges[0].pfns,
> +	WARN_ONCE(!gtt->range || !gtt->range->pfns,
>   		"No user pages to check\n");
>   
> -	if (gtt->ranges) {
> -		for (i = 0; i < gtt->nr_ranges; i++)
> -			r |= hmm_vma_range_done(&gtt->ranges[i]);
> -		kvfree(gtt->ranges[0].pfns);
> -		kvfree(gtt->ranges);
> -		gtt->ranges = NULL;
> +	if (gtt->range) {
> +		r = hmm_range_valid(gtt->range);
> +		hmm_range_unregister(gtt->range);
> +
> +		kvfree(gtt->range->pfns);
> +		kvfree(gtt->range);
> +		gtt->range = NULL;
>   	}
>   
>   	return r;
> @@ -953,9 +919,9 @@ static void amdgpu_ttm_tt_unpin_userptr(struct ttm_tt *ttm)
>   	sg_free_table(ttm->sg);
>   
>   #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
> -	if (gtt->ranges &&
> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
> -					     gtt->ranges[0].pfns[0]))
> +	if (gtt->range &&
> +	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
> +						      gtt->range->pfns[0]))

I think just checking gtt->range here is enough. If gtt->range is not 
NULL here, we're leaking memory.

Regards,
   Felix


>   		WARN_ONCE(1, "Missing get_user_page_done\n");
>   #endif
>   }
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Yang, Philip]

On 2019-05-30 6:36 p.m., Kuehling, Felix wrote:
>>    
>>    #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>> -	if (gtt->ranges &&
>> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>> -					     gtt->ranges[0].pfns[0]))
>> +	if (gtt->range &&
>> +	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>> +						      gtt->range->pfns[0]))
> I think just checking gtt->range here is enough. If gtt->range is not
> NULL here, we're leaking memory.
> 
If just checking gtt->range, there is a false warning in amdgpu_test 
userptr case in amdgpu_cs_list_validate path. If userptr is invalidated, 
then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it 
goes to ttm_tt_unbind first to unpin old pages (this causes false 
warning) then call amdgpu_ttm_tt_set_user_pages.

I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN. 
use kzalloc to allocate small size range.

Thanks,
Philip

> Regards,
>     Felix
> 
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Kuehling, Felix]

On 2019-05-31 1:28 p.m., Yang, Philip wrote:
>
> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote:
>>>     
>>>     #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>> -	if (gtt->ranges &&
>>> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>>> -					     gtt->ranges[0].pfns[0]))
>>> +	if (gtt->range &&
>>> +	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>>> +						      gtt->range->pfns[0]))
>> I think just checking gtt->range here is enough. If gtt->range is not
>> NULL here, we're leaking memory.
>>
> If just checking gtt->range, there is a false warning in amdgpu_test
> userptr case in amdgpu_cs_list_validate path. If userptr is invalidated,
> then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it
> goes to ttm_tt_unbind first to unpin old pages (this causes false
> warning) then call amdgpu_ttm_tt_set_user_pages.

But doesn't that mean we're leaking the gtt->range somewhere?

Regards,
   Felix


>
> I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN.
> use kzalloc to allocate small size range.
>
> Thanks,
> Philip
>
>> Regards,
>>      Felix
>>
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Yang, Philip]

On 2019-05-31 3:42 p.m., Kuehling, Felix wrote:
> On 2019-05-31 1:28 p.m., Yang, Philip wrote:
>>
>> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote:
>>>>      
>>>>      #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>> -	if (gtt->ranges &&
>>>> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>>>> -					     gtt->ranges[0].pfns[0]))
>>>> +	if (gtt->range &&
>>>> +	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>>>> +						      gtt->range->pfns[0]))
>>> I think just checking gtt->range here is enough. If gtt->range is not
>>> NULL here, we're leaking memory.
>>>
>> If just checking gtt->range, there is a false warning in amdgpu_test
>> userptr case in amdgpu_cs_list_validate path. If userptr is invalidated,
>> then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it
>> goes to ttm_tt_unbind first to unpin old pages (this causes false
>> warning) then call amdgpu_ttm_tt_set_user_pages.
> 
> But doesn't that mean we're leaking the gtt->range somewhere?
> 
ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate, 
the later one causes false warning. ttm_ttm_destory path is fine to only 
check gtt->range.

Double checked, amdgpu_ttm_tt_get_user_pages and 
amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak 
gtt->range.

1. amdgpu_gem_userptr_ioctl
       amdgpu_ttm_tt_get_user_pages
           ttm->pages for userptr pages
       amdgpu_ttm_tt_get_user_pages_done

2. amdgpu_cs_ioctl
       amdgpu_cs_parser_bos
           amdgpu_ttm_tt_get_user_pages
           if (userpage_invalidated)
               e->user_pages for new pages
           amdgpu_cs_list_validate
               if (userpage_invalidated)
                  ttm_tt_unbind ttm->pages // this causes warning
                  amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages)
       amdgpu_cs_submit
           amdgpu_ttm_tt_get_user_pages_done

> Regards,
>     Felix
> 
> 
>>

>> I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN.
>> use kzalloc to allocate small size range.
>>
>> Thanks,
>> Philip
>>
>>> Regards,
>>>       Felix
>>>
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Kuehling, Felix]

On 2019-05-31 5:32 p.m., Yang, Philip wrote:
>
> On 2019-05-31 3:42 p.m., Kuehling, Felix wrote:
>> On 2019-05-31 1:28 p.m., Yang, Philip wrote:
>>> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote:
>>>>>       
>>>>>       #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>>> -	if (gtt->ranges &&
>>>>> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>>>>> -					     gtt->ranges[0].pfns[0]))
>>>>> +	if (gtt->range &&
>>>>> +	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>>>>> +						      gtt->range->pfns[0]))
>>>> I think just checking gtt->range here is enough. If gtt->range is not
>>>> NULL here, we're leaking memory.
>>>>
>>> If just checking gtt->range, there is a false warning in amdgpu_test
>>> userptr case in amdgpu_cs_list_validate path. If userptr is invalidated,
>>> then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it
>>> goes to ttm_tt_unbind first to unpin old pages (this causes false
>>> warning) then call amdgpu_ttm_tt_set_user_pages.
>> But doesn't that mean we're leaking the gtt->range somewhere?
>>
> ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate,
> the later one causes false warning. ttm_ttm_destory path is fine to only
> check gtt->range.
>
> Double checked, amdgpu_ttm_tt_get_user_pages and
> amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak
> gtt->range.
>
> 1. amdgpu_gem_userptr_ioctl
>         amdgpu_ttm_tt_get_user_pages
>             ttm->pages for userptr pages
>         amdgpu_ttm_tt_get_user_pages_done
>
> 2. amdgpu_cs_ioctl
>         amdgpu_cs_parser_bos
>             amdgpu_ttm_tt_get_user_pages
>             if (userpage_invalidated)
>                 e->user_pages for new pages
>             amdgpu_cs_list_validate
>                 if (userpage_invalidated)
>                    ttm_tt_unbind ttm->pages // this causes warning
>                    amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages)

Hmm, I think amdgpu_cs is doing something weird there. It does some 
double book-keeping of the user pages in the BO list and the TTM BO. We 
did something similar for KFD and simplified it when moving to HMM. It 
could probably be simplified for amdgpu_cs as well. But not in this 
patch series.

I'll review your updated change.

Thanks,
   Felix


>         amdgpu_cs_submit
>             amdgpu_ttm_tt_get_user_pages_done
>
>> Regards,
>>      Felix
>>
>>
>>> I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN.
>>> use kzalloc to allocate small size range.
>>>
>>> Thanks,
>>> Philip
>>>
>>>> Regards,
>>>>        Felix
>>>>
>>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Christian König]

Am 01.06.19 um 00:01 schrieb Kuehling, Felix:
> On 2019-05-31 5:32 p.m., Yang, Philip wrote:
>> On 2019-05-31 3:42 p.m., Kuehling, Felix wrote:
>>> On 2019-05-31 1:28 p.m., Yang, Philip wrote:
>>>> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote:
>>>>>>        
>>>>>>        #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>>>> -	if (gtt->ranges &&
>>>>>> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>>>>>> -					     gtt->ranges[0].pfns[0]))
>>>>>> +	if (gtt->range &&
>>>>>> +	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>>>>>> +						      gtt->range->pfns[0]))
>>>>> I think just checking gtt->range here is enough. If gtt->range is not
>>>>> NULL here, we're leaking memory.
>>>>>
>>>> If just checking gtt->range, there is a false warning in amdgpu_test
>>>> userptr case in amdgpu_cs_list_validate path. If userptr is invalidated,
>>>> then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it
>>>> goes to ttm_tt_unbind first to unpin old pages (this causes false
>>>> warning) then call amdgpu_ttm_tt_set_user_pages.
>>> But doesn't that mean we're leaking the gtt->range somewhere?
>>>
>> ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate,
>> the later one causes false warning. ttm_ttm_destory path is fine to only
>> check gtt->range.
>>
>> Double checked, amdgpu_ttm_tt_get_user_pages and
>> amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak
>> gtt->range.
>>
>> 1. amdgpu_gem_userptr_ioctl
>>          amdgpu_ttm_tt_get_user_pages
>>              ttm->pages for userptr pages
>>          amdgpu_ttm_tt_get_user_pages_done
>>
>> 2. amdgpu_cs_ioctl
>>          amdgpu_cs_parser_bos
>>              amdgpu_ttm_tt_get_user_pages
>>              if (userpage_invalidated)
>>                  e->user_pages for new pages
>>              amdgpu_cs_list_validate
>>                  if (userpage_invalidated)
>>                     ttm_tt_unbind ttm->pages // this causes warning
>>                     amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages)
> Hmm, I think amdgpu_cs is doing something weird there. It does some
> double book-keeping of the user pages in the BO list and the TTM BO. We
> did something similar for KFD and simplified it when moving to HMM. It
> could probably be simplified for amdgpu_cs as well. But not in this
> patch series.

That actually sounds like a bug to me.

It is most likely a leftover from the time when we couldn't get the 
pages for a BO while the BO was reserved.

Going to take a closer look,
Christian.

>
> I'll review your updated change.
>
> Thanks,
>     Felix
>
>
>>          amdgpu_cs_submit
>>              amdgpu_ttm_tt_get_user_pages_done
>>
>>> Regards,
>>>       Felix
>>>
>>>
>>>> I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN.
>>>> use kzalloc to allocate small size range.
>>>>
>>>> Thanks,
>>>> Philip
>>>>
>>>>> Regards,
>>>>>         Felix
>>>>>
>>>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Christian König]

Am 03.06.19 um 12:17 schrieb Christian König:
> Am 01.06.19 um 00:01 schrieb Kuehling, Felix:
>> On 2019-05-31 5:32 p.m., Yang, Philip wrote:
>>> On 2019-05-31 3:42 p.m., Kuehling, Felix wrote:
>>>> On 2019-05-31 1:28 p.m., Yang, Philip wrote:
>>>>> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote:
>>>>>>>               #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>>>>> -    if (gtt->ranges &&
>>>>>>> -        ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>>>>>>> -                         gtt->ranges[0].pfns[0]))
>>>>>>> +    if (gtt->range &&
>>>>>>> +        ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>>>>>>> + gtt->range->pfns[0]))
>>>>>> I think just checking gtt->range here is enough. If gtt->range is 
>>>>>> not
>>>>>> NULL here, we're leaking memory.
>>>>>>
>>>>> If just checking gtt->range, there is a false warning in amdgpu_test
>>>>> userptr case in amdgpu_cs_list_validate path. If userptr is 
>>>>> invalidated,
>>>>> then ttm->pages[0] is outdated pages, lobj->user_pages is new 
>>>>> pages, it
>>>>> goes to ttm_tt_unbind first to unpin old pages (this causes false
>>>>> warning) then call amdgpu_ttm_tt_set_user_pages.
>>>> But doesn't that mean we're leaking the gtt->range somewhere?
>>>>
>>> ttm_tt_unbind is called from ttm_tt_destroy and 
>>> amdgpu_cs_list_validate,
>>> the later one causes false warning. ttm_ttm_destory path is fine to 
>>> only
>>> check gtt->range.
>>>
>>> Double checked, amdgpu_ttm_tt_get_user_pages and
>>> amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no 
>>> leak
>>> gtt->range.
>>>
>>> 1. amdgpu_gem_userptr_ioctl
>>>          amdgpu_ttm_tt_get_user_pages
>>>              ttm->pages for userptr pages
>>>          amdgpu_ttm_tt_get_user_pages_done
>>>
>>> 2. amdgpu_cs_ioctl
>>>          amdgpu_cs_parser_bos
>>>              amdgpu_ttm_tt_get_user_pages
>>>              if (userpage_invalidated)
>>>                  e->user_pages for new pages
>>>              amdgpu_cs_list_validate
>>>                  if (userpage_invalidated)
>>>                     ttm_tt_unbind ttm->pages // this causes warning
>>> amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages)
>> Hmm, I think amdgpu_cs is doing something weird there. It does some
>> double book-keeping of the user pages in the BO list and the TTM BO. We
>> did something similar for KFD and simplified it when moving to HMM. It
>> could probably be simplified for amdgpu_cs as well. But not in this
>> patch series.
>
> That actually sounds like a bug to me.
>
> It is most likely a leftover from the time when we couldn't get the 
> pages for a BO while the BO was reserved.

Mhm, at least it's not racy in the way I thought it would be. But it is 
certainly still overkill and should be simplified.

Philip are you taking a look or should I tackle this?

Thanks,
Christian.

>
>
> Going to take a closer look,
> Christian.
>
>>
>> I'll review your updated change.
>>
>> Thanks,
>>     Felix
>>
>>
>>>          amdgpu_cs_submit
>>>              amdgpu_ttm_tt_get_user_pages_done
>>>
>>>> Regards,
>>>>       Felix
>>>>
>>>>
>>>>> I will submit patch v2, to add retry if hmm_range_fault returns 
>>>>> -EAGAIN.
>>>>> use kzalloc to allocate small size range.
>>>>>
>>>>> Thanks,
>>>>> Philip
>>>>>
>>>>>> Regards,
>>>>>>         Felix
>>>>>>
>>>>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

[Yang, Philip]

On 2019-06-03 7:23 a.m., Christian König wrote:
> Am 03.06.19 um 12:17 schrieb Christian König:
>> Am 01.06.19 um 00:01 schrieb Kuehling, Felix:
>>> On 2019-05-31 5:32 p.m., Yang, Philip wrote:
>>>> On 2019-05-31 3:42 p.m., Kuehling, Felix wrote:
>>>>> On 2019-05-31 1:28 p.m., Yang, Philip wrote:
>>>>>> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote:
>>>>>>>>               #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>>>>>> -    if (gtt->ranges &&
>>>>>>>> -        ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>>>>>>>> -                         gtt->ranges[0].pfns[0]))
>>>>>>>> +    if (gtt->range &&
>>>>>>>> +        ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>>>>>>>> + gtt->range->pfns[0]))
>>>>>>> I think just checking gtt->range here is enough. If gtt->range is 
>>>>>>> not
>>>>>>> NULL here, we're leaking memory.
>>>>>>>
>>>>>> If just checking gtt->range, there is a false warning in amdgpu_test
>>>>>> userptr case in amdgpu_cs_list_validate path. If userptr is 
>>>>>> invalidated,
>>>>>> then ttm->pages[0] is outdated pages, lobj->user_pages is new 
>>>>>> pages, it
>>>>>> goes to ttm_tt_unbind first to unpin old pages (this causes false
>>>>>> warning) then call amdgpu_ttm_tt_set_user_pages.
>>>>> But doesn't that mean we're leaking the gtt->range somewhere?
>>>>>
>>>> ttm_tt_unbind is called from ttm_tt_destroy and 
>>>> amdgpu_cs_list_validate,
>>>> the later one causes false warning. ttm_ttm_destory path is fine to 
>>>> only
>>>> check gtt->range.
>>>>
>>>> Double checked, amdgpu_ttm_tt_get_user_pages and
>>>> amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no 
>>>> leak
>>>> gtt->range.
>>>>
>>>> 1. amdgpu_gem_userptr_ioctl
>>>>          amdgpu_ttm_tt_get_user_pages
>>>>              ttm->pages for userptr pages
>>>>          amdgpu_ttm_tt_get_user_pages_done
>>>>
>>>> 2. amdgpu_cs_ioctl
>>>>          amdgpu_cs_parser_bos
>>>>              amdgpu_ttm_tt_get_user_pages
>>>>              if (userpage_invalidated)
>>>>                  e->user_pages for new pages
>>>>              amdgpu_cs_list_validate
>>>>                  if (userpage_invalidated)
>>>>                     ttm_tt_unbind ttm->pages // this causes warning
>>>> amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages)
>>> Hmm, I think amdgpu_cs is doing something weird there. It does some
>>> double book-keeping of the user pages in the BO list and the TTM BO. We
>>> did something similar for KFD and simplified it when moving to HMM. It
>>> could probably be simplified for amdgpu_cs as well. But not in this
>>> patch series.
>>
>> That actually sounds like a bug to me.
>>
>> It is most likely a leftover from the time when we couldn't get the 
>> pages for a BO while the BO was reserved.
> 
> Mhm, at least it's not racy in the way I thought it would be. But it is 
> certainly still overkill and should be simplified.
> 
> Philip are you taking a look or should I tackle this?
> 
Hi Christian,

I will submit another patch to simplify amdgpu_cs_ioctl path, please 
help review it.

Thanks,
Philip

> Thanks,
> Christian.
> 
>>
>>
>> Going to take a closer look,
>> Christian.
>>
>>>
>>> I'll review your updated change.
>>>
>>> Thanks,
>>>     Felix
>>>
>>>
>>>>          amdgpu_cs_submit
>>>>              amdgpu_ttm_tt_get_user_pages_done
>>>>
>>>>> Regards,
>>>>>       Felix
>>>>>
>>>>>
>>>>>> I will submit patch v2, to add retry if hmm_range_fault returns 
>>>>>> -EAGAIN.
>>>>>> use kzalloc to allocate small size range.
>>>>>>
>>>>>> Thanks,
>>>>>> Philip
>>>>>>
>>>>>>> Regards,
>>>>>>>         Felix
>>>>>>>
>>>>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx