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

Joonas Lahtinen
2016-04-05T10:01:39Z

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