[Intel-gfx] [PATCH 2/2] drm/i915: Move stale context reaping to common i915_gem_context_create

[Tvrtko Ursulin]

On 05/07/2017 14:18, Chris Wilson wrote:
> We need to reap the stale contexts for all new contexts, be they created
> by user in i915_gem_context_ioctl or from opening a new file in
> i915_gem_context_open. Both paths may be called very frequently
> accumulating many stale contexts before any worker has a chance to run
> and free their memory.
> 
> Fixes: 1acfc104cdf8 ("drm/i915: Enable rcu-only context lookups")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 98d2ce98f467..c58a95c33c25 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -383,6 +383,10 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
>   
>   	lockdep_assert_held(&dev_priv->drm.struct_mutex);
>   
> +	/* Reap stale contexts */
> +	i915_gem_retire_requests(dev_priv);
> +	contexts_free(dev_priv);
> +
>   	ctx = __create_hw_context(dev_priv, file_priv);
>   	if (IS_ERR(ctx))
>   		return ctx;
> @@ -989,10 +993,6 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>   	if (ret)
>   		return ret;
>   
> -	/* Reap stale contexts */
> -	i915_gem_retire_requests(dev_priv);
> -	contexts_free(dev_priv);
> -
>   	ctx = i915_gem_create_context(dev_priv, file_priv);
>   	mutex_unlock(&dev->struct_mutex);
>   	if (IS_ERR(ctx))
> 

Can't make myself concerned about the context creation slowdown with 
this solution. I guess it highlights the same old RCU/delayed worker 
issues, but it could be an option to somehow generalize these things 
based on some threshold, like:

maybe_pruge_deleted_contexts()
{
	if (nr_contexts_on_free_list > some_threshold) {
		retire...
		free...
	}
}

Anyway:

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

Regards,

Tvrtko