Re: [Intel-gfx] [PATCH 2/8] drm/i915: Avoid live-lock with i915_vma_parked()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 23/03/2020 09:28, Chris Wilson wrote:
> Abuse^W Take advantage that we know we are inside the GT wakeref and
> that prevents any client execbuf from reopening the i915_vma in order to
> claim all the vma to close without having to drop the spinlock to free
> each one individually. By keeping the spinlock, we do not have to
> restart if we run concurrently with i915_gem_free_objects -- which
> causes them both to restart continually and make very very slow
> progress.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/1361
> Fixes: 77853186e547 ("drm/i915: Claim vma while under closed_lock in i915_vma_parked()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_vma.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 5b3efb43a8ef..08699fa069aa 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -1097,6 +1097,7 @@ void i915_vma_release(struct kref *ref)
>   void i915_vma_parked(struct intel_gt *gt)
>   {
>   	struct i915_vma *vma, *next;
> +	LIST_HEAD(closed);
>   
>   	spin_lock_irq(&gt->closed_lock);
>   	list_for_each_entry_safe(vma, next, &gt->closed_vma, closed_link) {
> @@ -1108,28 +1109,26 @@ void i915_vma_parked(struct intel_gt *gt)
>   		if (!kref_get_unless_zero(&obj->base.refcount))
>   			continue;
>   
> -		if (i915_vm_tryopen(vm)) {
> -			list_del_init(&vma->closed_link);
> -		} else {
> +		if (!i915_vm_tryopen(vm)) {
>   			i915_gem_object_put(obj);
> -			obj = NULL;
> +			continue;
>   		}
>   
> -		spin_unlock_irq(&gt->closed_lock);
> +		list_move(&vma->closed_link, &closed);
> +	}
> +	spin_unlock_irq(&gt->closed_lock);
>   
> -		if (obj) {
> -			__i915_vma_put(vma);
> -			i915_gem_object_put(obj);
> -		}
> +	/* As the GT is held idle, no vma can be reopened as we destroy them */
> +	list_for_each_entry_safe(vma, next, &closed, closed_link) {
> +		struct drm_i915_gem_object *obj = vma->obj;
> +		struct i915_address_space *vm = vma->vm;
>   
> -		i915_vm_close(vm);
> +		INIT_LIST_HEAD(&vma->closed_link);
> +		__i915_vma_put(vma);
>   
> -		/* Restart after dropping lock */
> -		spin_lock_irq(&gt->closed_lock);
> -		next = list_first_entry(&gt->closed_vma,
> -					typeof(*next), closed_link);
> +		i915_gem_object_put(obj);
> +		i915_vm_close(vm);
>   	}
> -	spin_unlock_irq(&gt->closed_lock);
>   }
>   
>   static void __i915_vma_iounmap(struct i915_vma *vma)
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx