Re: [PATCH] drm/amdgpu: stop allocating PDs/PTs with the eviction lock held

[Felix Kuehling <felix.kuehling@amd.com>]

Now you allow eviction of page tables while you allocate page tables. 
Isn't the whole point of the eviction lock to prevent page table 
evictions while manipulating page tables?

Or does this only apply to PTE invalidations which never allocated 
memory? Is that the only case that doesn't reserve page tables?

Regards,
   Felix

On 2020-02-12 10:14 a.m., Christian König wrote:
> We need to make sure to not allocate PDs/PTs while holding
> the eviction lock or otherwise we will run into lock inversion
> in the MM as soon as we enable the MMU notifier.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 31 +++++++++++++++++++++-----
>   1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 77c400675b79..e7ab0c1e2793 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -897,27 +897,42 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   	struct amdgpu_vm_pt *entry = cursor->entry;
>   	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *pt;
> +	bool need_entries;
>   	int r;
>   
> -	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
> +	need_entries = cursor->level < AMDGPU_VM_PTB && !entry->entries;
> +	if (!need_entries && entry->base.bo)
> +		return 0;
> +
> +	/* We need to make sure that we don't allocate PDs/PTs while holding the
> +	 * eviction lock or we run into lock recursion in the MM.
> +	 */
> +	amdgpu_vm_eviction_unlock(vm);
> +
> +	if (need_entries) {
>   		unsigned num_entries;
>   
>   		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>   		entry->entries = kvmalloc_array(num_entries,
>   						sizeof(*entry->entries),
>   						GFP_KERNEL | __GFP_ZERO);
> -		if (!entry->entries)
> -			return -ENOMEM;
> +		if (!entry->entries) {
> +			r = -ENOMEM;
> +			goto error_lock;
> +		}
>   	}
>   
> -	if (entry->base.bo)
> -		return 0;
> +	if (entry->base.bo) {
> +		r = 0;
> +		goto error_lock;
> +	}
>   
>   	amdgpu_vm_bo_param(adev, vm, cursor->level, direct, &bp);
>   
>   	r = amdgpu_bo_create(adev, &bp, &pt);
> +	amdgpu_vm_eviction_lock(vm);
>   	if (r)
> -		return r;
> +		goto error_free_pt;
>   
>   	/* Keep a reference to the root directory to avoid
>   	 * freeing them up in the wrong order.
> @@ -936,6 +951,10 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   	amdgpu_bo_unref(&pt);
>   	entry->base.bo = NULL;
>   	return r;
> +
> +error_lock:
> +	amdgpu_vm_eviction_lock(vm);
> +	return r;
>   }
>   
>   /**
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx