Re: [PATCH] drm/i915: Protect i915_active iterators from the shrinker

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

On 08/02/2019 13:47, Chris Wilson wrote:
> If we allocate while iterating the rbtree of active nodes, we may hit
> the shrinker and so retire the i915_active reap the rbtree. Modifying
> the rbtree as we iterate is not good behaviour, so acquire the
> i915_active first to keep the tree intact whenever we allocate.
> 
> Fixes: a42375af0a30 ("drm/i915: Release the active tracker tree upon idling")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_active.c | 36 +++++++++++++++++++++---------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 215b6ff8aa73..db7bb5bd5add 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -163,17 +163,25 @@ int i915_active_ref(struct i915_active *ref,
>   		    struct i915_request *rq)
>   {
>   	struct i915_active_request *active;
> +	int err = 0;
> +
> +	/* Prevent reaping in case we malloc/wait while building the tree */
> +	i915_active_acquire(ref);
>   
>   	active = active_instance(ref, timeline);
> -	if (IS_ERR(active))
> -		return PTR_ERR(active);
> +	if (IS_ERR(active)) {
> +		err = PTR_ERR(active);
> +		goto out;
> +	}
>   
>   	if (!i915_active_request_isset(active))
>   		ref->count++;
>   	__i915_active_request_set(active, rq);
>   
>   	GEM_BUG_ON(!ref->count);
> -	return 0;
> +out:
> +	i915_active_release(ref);
> +	return err;
>   }
>   
>   bool i915_active_acquire(struct i915_active *ref)
> @@ -223,19 +231,25 @@ int i915_request_await_active_request(struct i915_request *rq,
>   int i915_request_await_active(struct i915_request *rq, struct i915_active *ref)
>   {
>   	struct active_node *it, *n;
> -	int ret;
> +	int err = 0;
>   
> -	ret = i915_request_await_active_request(rq, &ref->last);
> -	if (ret)
> -		return ret;
> +	/* await allocates and so we need to avoid hitting the shrinker */
> +	if (i915_active_acquire(ref))
> +		goto out; /* was idle */
> +
> +	err = i915_request_await_active_request(rq, &ref->last);
> +	if (err)
> +		goto out;
>   
>   	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
> -		ret = i915_request_await_active_request(rq, &it->base);
> -		if (ret)
> -			return ret;
> +		err = i915_request_await_active_request(rq, &it->base);
> +		if (err)
> +			goto out;
>   	}
>   
> -	return 0;
> +out:
> +	i915_active_release(ref);
> +	return err;
>   }
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
> 

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