Re: [PATCH] drm/i915/shrinker: Refactor common uninterruptible locking

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ti, 2016-04-05 at 10:22 +0100, Chris Wilson wrote:
> Both the oom and vmap notifier callbacks have a loop to acquire the
> struct_mutex and set the device as uninterruptible, within a certain
> time. Refactor the common code into a pair of functions.
> 
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Looks good.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 79 +++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index be7501afb59e..39943793edcc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -289,35 +289,56 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
>  	return freed;
>  }
>  
> +struct shrinker_lock_uninterruptible {
> +	bool was_interruptible;
> +	bool unlock;
> +};
> +
> +static bool
> +i915_gem_shrinker_lock_uninterruptible(struct drm_i915_private *dev_priv,
> +				       struct shrinker_lock_uninterruptible *slu,
> +				       int timeout_ms)
> +{
> +	unsigned long timeout = msecs_to_jiffies(timeout_ms) + 1;
> +
> +	while (!i915_gem_shrinker_lock(dev_priv->dev, &slu->unlock)) {
> +		schedule_timeout_killable(1);
> +		if (fatal_signal_pending(current))
> +			return false;
> +		if (--timeout == 0) {
> +			pr_err("Unable to lock GPU to purge memory.\n");
> +			return false;
> +		}
> +	}
> +
> +	slu->was_interruptible = dev_priv->mm.interruptible;
> +	dev_priv->mm.interruptible = false;
> +	return true;
> +}
> +
> +static void
> +i915_gem_shrinker_unlock_uninterruptible(struct drm_i915_private *dev_priv,
> +					 struct shrinker_lock_uninterruptible *slu)
> +{
> +	dev_priv->mm.interruptible = slu->was_interruptible;
> +	if (slu->unlock)
> +		mutex_unlock(&dev_priv->dev->struct_mutex);
> +}
> +
>  static int
>  i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>  {
>  	struct drm_i915_private *dev_priv =
>  		container_of(nb, struct drm_i915_private, mm.oom_notifier);
> -	struct drm_device *dev = dev_priv->dev;
> +	struct shrinker_lock_uninterruptible slu;
>  	struct drm_i915_gem_object *obj;
> -	unsigned long timeout = msecs_to_jiffies(5000) + 1;
>  	unsigned long pinned, bound, unbound, freed_pages;
> -	bool was_interruptible;
> -	bool unlock;
>  
> -	while (!i915_gem_shrinker_lock(dev, &unlock) && --timeout) {
> -		schedule_timeout_killable(1);
> -		if (fatal_signal_pending(current))
> -			return NOTIFY_DONE;
> -	}
> -	if (timeout == 0) {
> -		pr_err("Unable to purge GPU memory due lock contention.\n");
> +	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
>  		return NOTIFY_DONE;
> -	}
> -
> -	was_interruptible = dev_priv->mm.interruptible;
> -	dev_priv->mm.interruptible = false;
>  
>  	freed_pages = i915_gem_shrink_all(dev_priv);
>  
> -	dev_priv->mm.interruptible = was_interruptible;
> -
>  	/* Because we may be allocating inside our own driver, we cannot
>  	 * assert that there are no objects with pinned pages that are not
>  	 * being pointed to by hardware.
> @@ -342,8 +363,7 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>  			bound += obj->base.size;
>  	}
>  
> -	if (unlock)
> -		mutex_unlock(&dev->struct_mutex);
> +	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
>  
>  	if (freed_pages || unbound || bound)
>  		pr_info("Purging GPU memory, %lu bytes freed, %lu bytes still pinned.\n",
> @@ -362,30 +382,15 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
>  {
>  	struct drm_i915_private *dev_priv =
>  		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
> -	struct drm_device *dev = dev_priv->dev;
> -	unsigned long timeout = msecs_to_jiffies(5000) + 1;
> +	struct shrinker_lock_uninterruptible slu;
>  	unsigned long freed_pages;
> -	bool was_interruptible;
> -	bool unlock;
>  
> -	while (!i915_gem_shrinker_lock(dev, &unlock) && --timeout) {
> -		schedule_timeout_killable(1);
> -		if (fatal_signal_pending(current))
> -			return NOTIFY_DONE;
> -	}
> -	if (timeout == 0) {
> -		pr_err("Unable to purge GPU vmaps due to lock contention.\n");
> +	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
>  		return NOTIFY_DONE;
> -	}
> -
> -	was_interruptible = dev_priv->mm.interruptible;
> -	dev_priv->mm.interruptible = false;
>  
>  	freed_pages = i915_gem_shrink_all(dev_priv);
>  
> -	dev_priv->mm.interruptible = was_interruptible;
> -	if (unlock)
> -		mutex_unlock(&dev->struct_mutex);
> +	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
>  
>  	*(unsigned long *)ptr += freed_pages;
>  	return NOTIFY_DONE;
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx