[PATCH 1/3] drm/amdkfd: support concurrent userptr update for HMM v2

[PATCH 1/3] drm/amdkfd: support concurrent userptr update for HMM v2

[Yang, Philip]

Userptr restore may have concurrent userptr invalidation after
hmm_vma_fault adds the range to the hmm->ranges list, needs call
hmm_vma_range_done to remove the range from hmm->ranges list first,
then reschedule the restore worker. Otherwise hmm_vma_fault will add
same range to the list, this will cause loop in the list because
range->next point to range itself.

Add function untrack_invalid_user_pages to reduce code duplication.

Change-Id: I31407739dc10554f8e418c7a0e0415d3d95552f1
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 25 ++++++++++++++-----
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index d2e315f42dad..60d53b0b497a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1935,6 +1935,23 @@ static int update_invalid_user_pages(struct amdkfd_process_info *process_info,
 	return 0;
 }
 
+/* Remove invalid userptr BOs from hmm track list
+ *
+ * Stop HMM track the userptr update
+ */
+static void untrack_invalid_user_pages(struct amdkfd_process_info *process_info)
+{
+	struct kgd_mem *mem, *tmp_mem;
+	struct amdgpu_bo *bo;
+
+	list_for_each_entry_safe(mem, tmp_mem,
+				 &process_info->userptr_inval_list,
+				 validate_list.head) {
+		bo = mem->bo;
+		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
+	}
+}
+
 /* Validate invalid userptr BOs
  *
  * Validates BOs on the userptr_inval_list, and moves them back to the
@@ -2052,12 +2069,6 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info)
 out_free:
 	kfree(pd_bo_list_entries);
 out_no_mem:
-	list_for_each_entry_safe(mem, tmp_mem,
-				 &process_info->userptr_inval_list,
-				 validate_list.head) {
-		bo = mem->bo;
-		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
-	}
 
 	return ret;
 }
@@ -2122,7 +2133,9 @@ static void amdgpu_amdkfd_restore_userptr_worker(struct work_struct *work)
 		 * hanging. No point trying again.
 		 */
 	}
+
 unlock_out:
+	untrack_invalid_user_pages(process_info);
 	mutex_unlock(&process_info->lock);
 	mmput(mm);
 	put_task_struct(usertask);
-- 
2.17.1

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

[PATCH 2/3] drm/amdgpu: support userptr cross VMAs case with HMM v2

[Yang, Philip]

userptr may cross two VMAs if the forked child process (not call exec
after fork) malloc buffer, then free it, and then malloc larger size
buf, kerenl will create new VMA adjacent to old VMA which was cloned
from parent process, some pages of userptr are in the first VMA, the
rest pages are in the second VMA.

HMM expects range only have one VMA, loop over all VMAs in the address
range, create multiple ranges to handle this case. See
is_mergeable_anon_vma in mm/mmap.c for details.

Change-Id: I0ca8c77e28deabccc139906f9ffee04b7e383314
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 123 +++++++++++++++++-------
 1 file changed, 88 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 7cc0ba24369d..802bec7ef917 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -711,7 +711,8 @@ struct amdgpu_ttm_tt {
 	struct task_struct	*usertask;
 	uint32_t		userflags;
 #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
-	struct hmm_range	range;
+	struct hmm_range	*ranges;
+	int			nr_ranges;
 #endif
 };
 
@@ -723,62 +724,105 @@ struct amdgpu_ttm_tt {
  * once afterwards to stop HMM tracking
  */
 #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 end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
-	struct hmm_range *range = &gtt->range;
-	int r = 0, i;
+	unsigned long start = gtt->userptr;
+	unsigned long end = start + ttm->num_pages * PAGE_SIZE;
+	struct hmm_range *ranges;
+	struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
+	uint64_t *pfns, f;
+	int r = 0, i, nr_pages;
 
 	if (!mm) /* Happens during process shutdown */
 		return -ESRCH;
 
-	amdgpu_hmm_init_range(range);
-
 	down_read(&mm->mmap_sem);
 
-	range->vma = find_vma(mm, gtt->userptr);
-	if (!range_in_vma(range->vma, gtt->userptr, end))
-		r = -EFAULT;
-	else if ((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
-		range->vma->vm_file)
+	/* user pages may cross multiple VMAs */
+	gtt->nr_ranges = 0;
+	do {
+		if (gtt->nr_ranges >= MAX_NR_VMAS) {
+			DRM_ERROR("Too many VMAs in userptr range\n");
+			r = -EFAULT;
+			goto out;
+		}
+
+		vma = find_vma(mm, vma ? vma->vm_end : start);
+		if (unlikely(!vma)) {
+			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);
+
+	if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
+		vmas[0]->vm_file)) {
 		r = -EPERM;
-	if (r)
 		goto out;
+	}
 
-	range->pfns = kvmalloc_array(ttm->num_pages, sizeof(uint64_t),
-				     GFP_KERNEL);
-	if (range->pfns == NULL) {
+	ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
+	if (unlikely(!ranges)) {
 		r = -ENOMEM;
 		goto out;
 	}
-	range->start = gtt->userptr;
-	range->end = end;
 
-	range->pfns[0] = range->flags[HMM_PFN_VALID];
-	range->pfns[0] |= amdgpu_ttm_tt_is_readonly(ttm) ?
-				0 : range->flags[HMM_PFN_WRITE];
-	for (i = 1; i < ttm->num_pages; i++)
-		range->pfns[i] = range->pfns[0];
+	pfns = kvmalloc_array(ttm->num_pages, sizeof(*pfns), GFP_KERNEL);
+	if (unlikely(!pfns)) {
+		r = -ENOMEM;
+		goto out_free_ranges;
+	}
+
+	for (i = 0; i < gtt->nr_ranges; i++)
+		amdgpu_hmm_init_range(&ranges[i]);
+
+	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;
+
+		r = hmm_vma_fault(&ranges[i], true);
+		if (unlikely(r))
+			break;
+	}
+	if (unlikely(r)) {
+		while (i--)
+			hmm_vma_range_done(&ranges[i]);
 
-	/* This may trigger page table update */
-	r = hmm_vma_fault(range, true);
-	if (r)
 		goto out_free_pfns;
+	}
 
 	up_read(&mm->mmap_sem);
 
 	for (i = 0; i < ttm->num_pages; i++)
-		pages[i] = hmm_pfn_to_page(range, range->pfns[i]);
+		pages[i] = hmm_pfn_to_page(&ranges[0], pfns[i]);
+	gtt->ranges = ranges;
 
 	return 0;
 
 out_free_pfns:
-	kvfree(range->pfns);
-	range->pfns = NULL;
+	kvfree(pfns);
+out_free_ranges:
+	kvfree(ranges);
 out:
 	up_read(&mm->mmap_sem);
+
 	return r;
 }
 
@@ -792,15 +836,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;
 
-	WARN_ONCE(!gtt->range.pfns, "No user pages to check\n");
-	if (gtt->range.pfns) {
-		r = hmm_vma_range_done(&gtt->range);
-		kvfree(gtt->range.pfns);
-		gtt->range.pfns = NULL;
+	DRM_DEBUG_DRIVER("user_pages_done 0x%llx nr_ranges %d pages 0x%lx\n",
+		gtt->userptr, gtt->nr_ranges, ttm->num_pages);
+
+	WARN_ONCE(!gtt->ranges || !gtt->ranges[0].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;
 	}
 
 	return r;
@@ -884,8 +936,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->range.pfns &&
-	    ttm->pages[0] == hmm_pfn_to_page(&gtt->range, gtt->range.pfns[0]))
+	if (gtt->ranges &&
+	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
+					     gtt->ranges[0].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 2/3] drm/amdgpu: support userptr cross VMAs case with HMM v2

[Kuehling, Felix]

See one comment inline. There are still some potential problems that 
you're not catching.

On 2019-03-06 9:42 p.m., Yang, Philip wrote:
> userptr may cross two VMAs if the forked child process (not call exec
> after fork) malloc buffer, then free it, and then malloc larger size
> buf, kerenl will create new VMA adjacent to old VMA which was cloned
> from parent process, some pages of userptr are in the first VMA, the
> rest pages are in the second VMA.
>
> HMM expects range only have one VMA, loop over all VMAs in the address
> range, create multiple ranges to handle this case. See
> is_mergeable_anon_vma in mm/mmap.c for details.
>
> Change-Id: I0ca8c77e28deabccc139906f9ffee04b7e383314
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 123 +++++++++++++++++-------
>   1 file changed, 88 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 7cc0ba24369d..802bec7ef917 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -711,7 +711,8 @@ struct amdgpu_ttm_tt {
>   	struct task_struct	*usertask;
>   	uint32_t		userflags;
>   #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
> -	struct hmm_range	range;
> +	struct hmm_range	*ranges;
> +	int			nr_ranges;
>   #endif
>   };
>   
> @@ -723,62 +724,105 @@ struct amdgpu_ttm_tt {
>    * once afterwards to stop HMM tracking
>    */
>   #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 end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
> -	struct hmm_range *range = &gtt->range;
> -	int r = 0, i;
> +	unsigned long start = gtt->userptr;
> +	unsigned long end = start + ttm->num_pages * PAGE_SIZE;
> +	struct hmm_range *ranges;
> +	struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
> +	uint64_t *pfns, f;
> +	int r = 0, i, nr_pages;
>   
>   	if (!mm) /* Happens during process shutdown */
>   		return -ESRCH;
>   
> -	amdgpu_hmm_init_range(range);
> -
>   	down_read(&mm->mmap_sem);
>   
> -	range->vma = find_vma(mm, gtt->userptr);
> -	if (!range_in_vma(range->vma, gtt->userptr, end))
> -		r = -EFAULT;
> -	else if ((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
> -		range->vma->vm_file)
> +	/* user pages may cross multiple VMAs */
> +	gtt->nr_ranges = 0;
> +	do {
> +		if (gtt->nr_ranges >= MAX_NR_VMAS) {
> +			DRM_ERROR("Too many VMAs in userptr range\n");
> +			r = -EFAULT;
> +			goto out;
> +		}
> +
> +		vma = find_vma(mm, vma ? vma->vm_end : start);

You need a check here that vma->vm_start <= the requested start address. 
Otherwise you can end up with gaps in your userptr mapping that don't 
have valid pages.

Regards,
   Felix


> +		if (unlikely(!vma)) {
> +			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);
> +
> +	if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
> +		vmas[0]->vm_file)) {
>   		r = -EPERM;
> -	if (r)
>   		goto out;
> +	}
>   
> -	range->pfns = kvmalloc_array(ttm->num_pages, sizeof(uint64_t),
> -				     GFP_KERNEL);
> -	if (range->pfns == NULL) {
> +	ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
> +	if (unlikely(!ranges)) {
>   		r = -ENOMEM;
>   		goto out;
>   	}
> -	range->start = gtt->userptr;
> -	range->end = end;
>   
> -	range->pfns[0] = range->flags[HMM_PFN_VALID];
> -	range->pfns[0] |= amdgpu_ttm_tt_is_readonly(ttm) ?
> -				0 : range->flags[HMM_PFN_WRITE];
> -	for (i = 1; i < ttm->num_pages; i++)
> -		range->pfns[i] = range->pfns[0];
> +	pfns = kvmalloc_array(ttm->num_pages, sizeof(*pfns), GFP_KERNEL);
> +	if (unlikely(!pfns)) {
> +		r = -ENOMEM;
> +		goto out_free_ranges;
> +	}
> +
> +	for (i = 0; i < gtt->nr_ranges; i++)
> +		amdgpu_hmm_init_range(&ranges[i]);
> +
> +	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;
> +
> +		r = hmm_vma_fault(&ranges[i], true);
> +		if (unlikely(r))
> +			break;
> +	}
> +	if (unlikely(r)) {
> +		while (i--)
> +			hmm_vma_range_done(&ranges[i]);
>   
> -	/* This may trigger page table update */
> -	r = hmm_vma_fault(range, true);
> -	if (r)
>   		goto out_free_pfns;
> +	}
>   
>   	up_read(&mm->mmap_sem);
>   
>   	for (i = 0; i < ttm->num_pages; i++)
> -		pages[i] = hmm_pfn_to_page(range, range->pfns[i]);
> +		pages[i] = hmm_pfn_to_page(&ranges[0], pfns[i]);
> +	gtt->ranges = ranges;
>   
>   	return 0;
>   
>   out_free_pfns:
> -	kvfree(range->pfns);
> -	range->pfns = NULL;
> +	kvfree(pfns);
> +out_free_ranges:
> +	kvfree(ranges);
>   out:
>   	up_read(&mm->mmap_sem);
> +
>   	return r;
>   }
>   
> @@ -792,15 +836,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;
>   
> -	WARN_ONCE(!gtt->range.pfns, "No user pages to check\n");
> -	if (gtt->range.pfns) {
> -		r = hmm_vma_range_done(&gtt->range);
> -		kvfree(gtt->range.pfns);
> -		gtt->range.pfns = NULL;
> +	DRM_DEBUG_DRIVER("user_pages_done 0x%llx nr_ranges %d pages 0x%lx\n",
> +		gtt->userptr, gtt->nr_ranges, ttm->num_pages);
> +
> +	WARN_ONCE(!gtt->ranges || !gtt->ranges[0].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;
>   	}
>   
>   	return r;
> @@ -884,8 +936,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->range.pfns &&
> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->range, gtt->range.pfns[0]))
> +	if (gtt->ranges &&
> +	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
> +					     gtt->ranges[0].pfns[0]))
>   		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 1/3] drm/amdkfd: support concurrent userptr update for HMM v2

[Kuehling, Felix]

On 2019-03-06 9:42 p.m., Yang, Philip wrote:
> Userptr restore may have concurrent userptr invalidation after
> hmm_vma_fault adds the range to the hmm->ranges list, needs call
> hmm_vma_range_done to remove the range from hmm->ranges list first,
> then reschedule the restore worker. Otherwise hmm_vma_fault will add
> same range to the list, this will cause loop in the list because
> range->next point to range itself.
>
> Add function untrack_invalid_user_pages to reduce code duplication.
>
> Change-Id: I31407739dc10554f8e418c7a0e0415d3d95552f1
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>

This patch is Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 25 ++++++++++++++-----
>   1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index d2e315f42dad..60d53b0b497a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1935,6 +1935,23 @@ static int update_invalid_user_pages(struct amdkfd_process_info *process_info,
>   	return 0;
>   }
>   
> +/* Remove invalid userptr BOs from hmm track list
> + *
> + * Stop HMM track the userptr update
> + */
> +static void untrack_invalid_user_pages(struct amdkfd_process_info *process_info)
> +{
> +	struct kgd_mem *mem, *tmp_mem;
> +	struct amdgpu_bo *bo;
> +
> +	list_for_each_entry_safe(mem, tmp_mem,
> +				 &process_info->userptr_inval_list,
> +				 validate_list.head) {
> +		bo = mem->bo;
> +		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> +	}
> +}
> +
>   /* Validate invalid userptr BOs
>    *
>    * Validates BOs on the userptr_inval_list, and moves them back to the
> @@ -2052,12 +2069,6 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info)
>   out_free:
>   	kfree(pd_bo_list_entries);
>   out_no_mem:
> -	list_for_each_entry_safe(mem, tmp_mem,
> -				 &process_info->userptr_inval_list,
> -				 validate_list.head) {
> -		bo = mem->bo;
> -		amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> -	}
>   
>   	return ret;
>   }
> @@ -2122,7 +2133,9 @@ static void amdgpu_amdkfd_restore_userptr_worker(struct work_struct *work)
>   		 * hanging. No point trying again.
>   		 */
>   	}
> +
>   unlock_out:
> +	untrack_invalid_user_pages(process_info);
>   	mutex_unlock(&process_info->lock);
>   	mmput(mm);
>   	put_task_struct(usertask);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amdgpu: support userptr cross VMAs case with HMM v2

[Yang, Philip]

Hi Felix,

Submitted v3 to fix the potential problems with invalid userptr.

Philip

On 2019-03-12 3:30 p.m., Kuehling, Felix wrote:
> See one comment inline. There are still some potential problems that
> you're not catching.
> 
> On 2019-03-06 9:42 p.m., Yang, Philip wrote:
>> userptr may cross two VMAs if the forked child process (not call exec
>> after fork) malloc buffer, then free it, and then malloc larger size
>> buf, kerenl will create new VMA adjacent to old VMA which was cloned
>> from parent process, some pages of userptr are in the first VMA, the
>> rest pages are in the second VMA.
>>
>> HMM expects range only have one VMA, loop over all VMAs in the address
>> range, create multiple ranges to handle this case. See
>> is_mergeable_anon_vma in mm/mmap.c for details.
>>
>> Change-Id: I0ca8c77e28deabccc139906f9ffee04b7e383314
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 123 +++++++++++++++++-------
>>    1 file changed, 88 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 7cc0ba24369d..802bec7ef917 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -711,7 +711,8 @@ struct amdgpu_ttm_tt {
>>    	struct task_struct	*usertask;
>>    	uint32_t		userflags;
>>    #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>> -	struct hmm_range	range;
>> +	struct hmm_range	*ranges;
>> +	int			nr_ranges;
>>    #endif
>>    };
>>    
>> @@ -723,62 +724,105 @@ struct amdgpu_ttm_tt {
>>     * once afterwards to stop HMM tracking
>>     */
>>    #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 end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
>> -	struct hmm_range *range = &gtt->range;
>> -	int r = 0, i;
>> +	unsigned long start = gtt->userptr;
>> +	unsigned long end = start + ttm->num_pages * PAGE_SIZE;
>> +	struct hmm_range *ranges;
>> +	struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
>> +	uint64_t *pfns, f;
>> +	int r = 0, i, nr_pages;
>>    
>>    	if (!mm) /* Happens during process shutdown */
>>    		return -ESRCH;
>>    
>> -	amdgpu_hmm_init_range(range);
>> -
>>    	down_read(&mm->mmap_sem);
>>    
>> -	range->vma = find_vma(mm, gtt->userptr);
>> -	if (!range_in_vma(range->vma, gtt->userptr, end))
>> -		r = -EFAULT;
>> -	else if ((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
>> -		range->vma->vm_file)
>> +	/* user pages may cross multiple VMAs */
>> +	gtt->nr_ranges = 0;
>> +	do {
>> +		if (gtt->nr_ranges >= MAX_NR_VMAS) {
>> +			DRM_ERROR("Too many VMAs in userptr range\n");
>> +			r = -EFAULT;
>> +			goto out;
>> +		}
>> +
>> +		vma = find_vma(mm, vma ? vma->vm_end : start);
> 
> You need a check here that vma->vm_start <= the requested start address.
> Otherwise you can end up with gaps in your userptr mapping that don't
> have valid pages.
> 
> Regards,
>     Felix
> 
> 
>> +		if (unlikely(!vma)) {
>> +			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);
>> +
>> +	if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
>> +		vmas[0]->vm_file)) {
>>    		r = -EPERM;
>> -	if (r)
>>    		goto out;
>> +	}
>>    
>> -	range->pfns = kvmalloc_array(ttm->num_pages, sizeof(uint64_t),
>> -				     GFP_KERNEL);
>> -	if (range->pfns == NULL) {
>> +	ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
>> +	if (unlikely(!ranges)) {
>>    		r = -ENOMEM;
>>    		goto out;
>>    	}
>> -	range->start = gtt->userptr;
>> -	range->end = end;
>>    
>> -	range->pfns[0] = range->flags[HMM_PFN_VALID];
>> -	range->pfns[0] |= amdgpu_ttm_tt_is_readonly(ttm) ?
>> -				0 : range->flags[HMM_PFN_WRITE];
>> -	for (i = 1; i < ttm->num_pages; i++)
>> -		range->pfns[i] = range->pfns[0];
>> +	pfns = kvmalloc_array(ttm->num_pages, sizeof(*pfns), GFP_KERNEL);
>> +	if (unlikely(!pfns)) {
>> +		r = -ENOMEM;
>> +		goto out_free_ranges;
>> +	}
>> +
>> +	for (i = 0; i < gtt->nr_ranges; i++)
>> +		amdgpu_hmm_init_range(&ranges[i]);
>> +
>> +	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;
>> +
>> +		r = hmm_vma_fault(&ranges[i], true);
>> +		if (unlikely(r))
>> +			break;
>> +	}
>> +	if (unlikely(r)) {
>> +		while (i--)
>> +			hmm_vma_range_done(&ranges[i]);
>>    
>> -	/* This may trigger page table update */
>> -	r = hmm_vma_fault(range, true);
>> -	if (r)
>>    		goto out_free_pfns;
>> +	}
>>    
>>    	up_read(&mm->mmap_sem);
>>    
>>    	for (i = 0; i < ttm->num_pages; i++)
>> -		pages[i] = hmm_pfn_to_page(range, range->pfns[i]);
>> +		pages[i] = hmm_pfn_to_page(&ranges[0], pfns[i]);
>> +	gtt->ranges = ranges;
>>    
>>    	return 0;
>>    
>>    out_free_pfns:
>> -	kvfree(range->pfns);
>> -	range->pfns = NULL;
>> +	kvfree(pfns);
>> +out_free_ranges:
>> +	kvfree(ranges);
>>    out:
>>    	up_read(&mm->mmap_sem);
>> +
>>    	return r;
>>    }
>>    
>> @@ -792,15 +836,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;
>>    
>> -	WARN_ONCE(!gtt->range.pfns, "No user pages to check\n");
>> -	if (gtt->range.pfns) {
>> -		r = hmm_vma_range_done(&gtt->range);
>> -		kvfree(gtt->range.pfns);
>> -		gtt->range.pfns = NULL;
>> +	DRM_DEBUG_DRIVER("user_pages_done 0x%llx nr_ranges %d pages 0x%lx\n",
>> +		gtt->userptr, gtt->nr_ranges, ttm->num_pages);
>> +
>> +	WARN_ONCE(!gtt->ranges || !gtt->ranges[0].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;
>>    	}
>>    
>>    	return r;
>> @@ -884,8 +936,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->range.pfns &&
>> -	    ttm->pages[0] == hmm_pfn_to_page(&gtt->range, gtt->range.pfns[0]))
>> +	if (gtt->ranges &&
>> +	    ttm->pages[0] == hmm_pfn_to_page(&gtt->ranges[0],
>> +					     gtt->ranges[0].pfns[0]))
>>    		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