* [PATCH] drm/i915: Wait for the mutex whilst the reaper runs @ 2012-10-10 11:07 Chris Wilson 2012-10-10 15:08 ` [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim Chris Wilson 2012-10-10 15:21 ` [PATCH] drm/i915: Wait for the mutex whilst the reaper runs Chris Wilson 0 siblings, 2 replies; 7+ messages in thread From: Chris Wilson @ 2012-10-10 11:07 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim 2012-10-10 11:07 [PATCH] drm/i915: Wait for the mutex whilst the reaper runs Chris Wilson @ 2012-10-10 15:08 ` Chris Wilson 2012-10-10 21:02 ` Daniel Vetter 2012-11-08 13:49 ` Ben Widawsky 2012-10-10 15:21 ` [PATCH] drm/i915: Wait for the mutex whilst the reaper runs Chris Wilson 1 sibling, 2 replies; 7+ messages in thread From: Chris Wilson @ 2012-10-10 15:08 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim 2012-10-10 15:08 ` [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim Chris Wilson @ 2012-10-10 21:02 ` Daniel Vetter 2012-11-08 13:49 ` Ben Widawsky 1 sibling, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2012-10-10 21:02 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim 2012-10-10 15:08 ` [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim Chris Wilson 2012-10-10 21:02 ` Daniel Vetter @ 2012-11-08 13:49 ` Ben Widawsky 2012-11-09 8:58 ` [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT Chris Wilson 1 sibling, 1 reply; 7+ messages in thread From: Ben Widawsky @ 2012-11-08 13:49 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT 2012-11-08 13:49 ` Ben Widawsky @ 2012-11-09 8:58 ` Chris Wilson 2012-11-20 9:48 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Chris Wilson @ 2012-11-09 8:58 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT 2012-11-09 8:58 ` [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT Chris Wilson @ 2012-11-20 9:48 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2012-11-20 9:48 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Wait for the mutex whilst the reaper runs 2012-10-10 11:07 [PATCH] drm/i915: Wait for the mutex whilst the reaper runs Chris Wilson 2012-10-10 15:08 ` [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim Chris Wilson @ 2012-10-10 15:21 ` Chris Wilson 1 sibling, 0 replies; 7+ messages in thread From: Chris Wilson @ 2012-10-10 15:21 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-11-20 9:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-10-10 11:07 [PATCH] drm/i915: Wait for the mutex whilst the reaper runs Chris Wilson 2012-10-10 15:08 ` [PATCH] drm/i915: Borrow our struct_mutex for the direct reclaim Chris Wilson 2012-10-10 21:02 ` Daniel Vetter 2012-11-08 13:49 ` Ben Widawsky 2012-11-09 8:58 ` [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT Chris Wilson 2012-11-20 9:48 ` Daniel Vetter 2012-10-10 15:21 ` [PATCH] drm/i915: Wait for the mutex whilst the reaper runs Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.