[PATCH] drm/i915: Wait for the mutex whilst the reaper runs

[PATCH] drm/i915: Wait for the mutex whilst the reaper runs

[Chris Wilson]

If the system is forced to start kswapd to recover pages, the system is
very starved. Fortunately, kswapd is its own process context and can
wait for the mutex.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ff2ea2b..7108493 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4600,8 +4600,16 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 	int nr_to_scan = sc->nr_to_scan;
 	int cnt;
 
-	if (!mutex_trylock(&dev->struct_mutex))
-		return 0;
+	/* Force reaping when kswapd runs and the system is starved of
+	 * free pages, Otherwise only opportunistically shrink our caches,
+	 * being wary of deadlocks.
+	 */
+	if (current_is_kswapd()) {
+		mutex_lock(&dev->struct_mutex);
+	} else {
+		if (!mutex_trylock(&dev->struct_mutex))
+			return 0;
+	}
 
 	if (nr_to_scan) {
 		nr_to_scan -= i915_gem_purge(dev_priv, nr_to_scan);
-- 
1.7.10.4

[PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim

[Chris Wilson]

If we have hit oom whilst holding our struct_mutex, then currently we
cannot reap our own GPU buffers which likely pin most of memory, making
an outright OOM more likely. So if we are running in direct reclaim and
already hold the mutex, attempt to free buffers knowing that the
original function can not continue until we return.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1d0cbfb..bed4084 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4589,6 +4589,18 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
 	spin_unlock(&file_priv->mm.lock);
 }
 
+static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
+{
+	if (!mutex_is_locked(mutex))
+		return false;
+
+#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_SMP)
+	return mutex->owner == task;
+#else
+	return false;
+#endif
+}
+
 static int
 i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 {
@@ -4599,10 +4611,15 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_i915_gem_object *obj;
 	int nr_to_scan = sc->nr_to_scan;
+	bool unlock = true;
 	int cnt;
 
-	if (!mutex_trylock(&dev->struct_mutex))
-		return 0;
+	if (!mutex_trylock(&dev->struct_mutex)) {
+		if (mutex_is_locked_by(&dev->struct_mutex, current))
+			unlock = false;
+		else
+			return 0;
+	}
 
 	if (nr_to_scan) {
 		nr_to_scan -= i915_gem_purge(dev_priv, nr_to_scan);
@@ -4618,6 +4635,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 		if (obj->pin_count == 0 && obj->pages_pin_count == 0)
 			cnt += obj->base.size >> PAGE_SHIFT;
 
-	mutex_unlock(&dev->struct_mutex);
+	if (unlock)
+		mutex_unlock(&dev->struct_mutex);
 	return cnt;
 }
-- 
1.7.10.4

Re: [PATCH] drm/i915: Wait for the mutex whilst the reaper runs

[Chris Wilson]

On Wed, 10 Oct 2012 12:07:30 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If the system is forced to start kswapd to recover pages, the system is
> very starved. Fortunately, kswapd is its own process context and can
> wait for the mutex.

Note this doesn't survive inspection with lockdep due to dependency upon
pfmemalloc_wait in the direct reclaim path - that is, it is possible for
__GFP_FS allocations to wait indefinitely upon kswapd (who in turn may
be waiting for the dev->struct_mutex held by the direct reclaimer). As
we don't have complete control over all allocations made whilst holding
the struct_mutex, we can't control the gfp_t to avoid the deadlock.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim

[Daniel Vetter]

On Wed, Oct 10, 2012 at 5:08 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If we have hit oom whilst holding our struct_mutex, then currently we
> cannot reap our own GPU buffers which likely pin most of memory, making
> an outright OOM more likely. So if we are running in direct reclaim and
> already hold the mutex, attempt to free buffers knowing that the
> original function can not continue until we return.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I've thought a bit about this, and I fear the implications. It's a
very neat trick, but now every memory alloc call could potentially
result in unpinned objects getting unbound and in active objects
getting retired. Previously we only needed to fear active objects
disappearing when calling retire_request (since that could drop the
last reference). With this chance we have many more places, and given
how often we e.g. fumbled the refcounting in the fence stealing code
I'm scared ...

/me needs to think more about this

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1d0cbfb..bed4084 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4589,6 +4589,18 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
>         spin_unlock(&file_priv->mm.lock);
>  }
>
> +static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
> +{
> +       if (!mutex_is_locked(mutex))
> +               return false;
> +
> +#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_SMP)
> +       return mutex->owner == task;
> +#else
> +       return false;
> +#endif
> +}
> +
>  static int
>  i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
>  {
> @@ -4599,10 +4611,15 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
>         struct drm_device *dev = dev_priv->dev;
>         struct drm_i915_gem_object *obj;
>         int nr_to_scan = sc->nr_to_scan;
> +       bool unlock = true;
>         int cnt;
>
> -       if (!mutex_trylock(&dev->struct_mutex))
> -               return 0;
> +       if (!mutex_trylock(&dev->struct_mutex)) {
> +               if (mutex_is_locked_by(&dev->struct_mutex, current))
> +                       unlock = false;
> +               else
> +                       return 0;
> +       }
>
>         if (nr_to_scan) {
>                 nr_to_scan -= i915_gem_purge(dev_priv, nr_to_scan);
> @@ -4618,6 +4635,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
>                 if (obj->pin_count == 0 && obj->pages_pin_count == 0)
>                         cnt += obj->base.size >> PAGE_SHIFT;
>
> -       mutex_unlock(&dev->struct_mutex);
> +       if (unlock)
> +               mutex_unlock(&dev->struct_mutex);
>         return cnt;
>  }
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim

[Ben Widawsky]

On Wed, 10 Oct 2012 16:08:59 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> If we have hit oom whilst holding our struct_mutex, then currently we
> cannot reap our own GPU buffers which likely pin most of memory, making
> an outright OOM more likely. So if we are running in direct reclaim and
> already hold the mutex, attempt to free buffers knowing that the
> original function can not continue until we return.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I will try to review this a bit further on the plane ride home. It's
quite a scary patch, but it fixes OOM destroying IGT runs, so it's
pretty necessary IMO.

Meanwhile:
Tested-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1d0cbfb..bed4084 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4589,6 +4589,18 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
>  	spin_unlock(&file_priv->mm.lock);
>  }
>  
> +static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
> +{
> +	if (!mutex_is_locked(mutex))
> +		return false;
> +
> +#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_SMP)
> +	return mutex->owner == task;
> +#else
> +	return false;
> +#endif
> +}
> +
>  static int
>  i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
>  {
> @@ -4599,10 +4611,15 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_i915_gem_object *obj;
>  	int nr_to_scan = sc->nr_to_scan;
> +	bool unlock = true;
>  	int cnt;
>  
> -	if (!mutex_trylock(&dev->struct_mutex))
> -		return 0;
> +	if (!mutex_trylock(&dev->struct_mutex)) {
> +		if (mutex_is_locked_by(&dev->struct_mutex, current))
> +			unlock = false;
> +		else
> +			return 0;
> +	}
>  
>  	if (nr_to_scan) {
>  		nr_to_scan -= i915_gem_purge(dev_priv, nr_to_scan);
> @@ -4618,6 +4635,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
>  		if (obj->pin_count == 0 && obj->pages_pin_count == 0)
>  			cnt += obj->base.size >> PAGE_SHIFT;
>  
> -	mutex_unlock(&dev->struct_mutex);
> +	if (unlock)
> +		mutex_unlock(&dev->struct_mutex);
>  	return cnt;
>  }



-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT

[Chris Wilson]

In the circumstances that the shrinker is allowed to steal the mutex
in order to reap pages, we need to be careful to prevent it operating on
the current object and shooting ourselves in the foot.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 159dca5..f225583 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3091,6 +3091,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 	if (ret)
 		return ret;
 
+	i915_gem_object_pin_pages(obj);
+
  search_free:
 	if (map_and_fenceable)
 		free_space =
@@ -3121,14 +3123,17 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 					       obj->cache_level,
 					       map_and_fenceable,
 					       nonblocking);
-		if (ret)
+		if (ret) {
+			i915_gem_object_unpin_pages(obj);
 			return ret;
+		}
 
 		goto search_free;
 	}
 	if (WARN_ON(!i915_gem_valid_gtt_space(dev,
 					      obj->gtt_space,
 					      obj->cache_level))) {
+		i915_gem_object_unpin_pages(obj);
 		drm_mm_put_block(obj->gtt_space);
 		obj->gtt_space = NULL;
 		return -EINVAL;
@@ -3137,6 +3142,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 
 	ret = i915_gem_gtt_prepare_object(obj);
 	if (ret) {
+		i915_gem_object_unpin_pages(obj);
 		drm_mm_put_block(obj->gtt_space);
 		obj->gtt_space = NULL;
 		return ret;
@@ -3159,6 +3165,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 
 	obj->map_and_fenceable = mappable && fenceable;
 
+	i915_gem_object_unpin_pages(obj);
 	trace_i915_gem_object_bind(obj, map_and_fenceable);
 	i915_gem_verify_gtt(dev);
 	return 0;
-- 
1.7.10.4

Re: [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT

[Daniel Vetter]

On Fri, Nov 09, 2012 at 08:58:50AM +0000, Chris Wilson wrote:
> In the circumstances that the shrinker is allowed to steal the mutex
> in order to reap pages, we need to be careful to prevent it operating on
> the current object and shooting ourselves in the foot.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok, I this is definitely the major one that I've thought of, but I think
I've found another place that need a similar treatment:

i915_gem_fault: In between bind_to_gtt and vm_insert_pfn we have a call to
get_fence, which might potentially call kmalloc due to a pending request.
I know, it's highly unlikely that this will ever matter, but I think if we
braket this with a pin/unpin, it will nicely serve as a reminder where to
reserve/unreserve and object, in case we ever switch to a per-object
locking scheme.

Otherwise I can't poke holes into this any more ...

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 159dca5..f225583 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3091,6 +3091,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  	if (ret)
>  		return ret;
>  
> +	i915_gem_object_pin_pages(obj);
> +
>   search_free:
>  	if (map_and_fenceable)
>  		free_space =
> @@ -3121,14 +3123,17 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  					       obj->cache_level,
>  					       map_and_fenceable,
>  					       nonblocking);
> -		if (ret)
> +		if (ret) {
> +			i915_gem_object_unpin_pages(obj);
>  			return ret;
> +		}
>  
>  		goto search_free;
>  	}
>  	if (WARN_ON(!i915_gem_valid_gtt_space(dev,
>  					      obj->gtt_space,
>  					      obj->cache_level))) {
> +		i915_gem_object_unpin_pages(obj);
>  		drm_mm_put_block(obj->gtt_space);
>  		obj->gtt_space = NULL;
>  		return -EINVAL;
> @@ -3137,6 +3142,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  
>  	ret = i915_gem_gtt_prepare_object(obj);
>  	if (ret) {
> +		i915_gem_object_unpin_pages(obj);
>  		drm_mm_put_block(obj->gtt_space);
>  		obj->gtt_space = NULL;
>  		return ret;
> @@ -3159,6 +3165,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  
>  	obj->map_and_fenceable = mappable && fenceable;
>  
> +	i915_gem_object_unpin_pages(obj);
>  	trace_i915_gem_object_bind(obj, map_and_fenceable);
>  	i915_gem_verify_gtt(dev);
>  	return 0;
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch