On 04/12/2018 14:15, Chris Wilson wrote:
Ignore trying to shrink from i915 if we fail to acquire the struct_mutex
in the shrinker while performing direct-reclaim. The trade-off being
(much) lower latency for non-i915 clients at an increased risk of being
unable to obtain a page from direct-reclaim without hitting the
oom-notifier. The proviso being that we still keep trying to hard
obtain the lock for oom so that we can reap under heavy memory pressure.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 4 ++--
drivers/gpu/drm/i915/i915_gem_shrinker.c | 24 +++++++++++-------------
2 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c5f01964f0fb..1cad218b71d3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2916,9 +2916,9 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
__i915_gem_object_unpin_pages(obj);
}
-enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock */
+enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock/struct_mutex */
I915_MM_NORMAL = 0,
- I915_MM_SHRINKER
+ I915_MM_SHRINKER /* called "recursively" from direct-reclaim-esque */
};
void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index ea90d3a0d511..d461f458f4af 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -36,7 +36,9 @@
#include "i915_drv.h"
#include "i915_trace.h"
-static bool shrinker_lock(struct drm_i915_private *i915, bool *unlock)
+static bool shrinker_lock(struct drm_i915_private *i915,
+ unsigned int flags,
+ bool *unlock)
{
switch (mutex_trylock_recursive(&i915->drm.struct_mutex)) {
case MUTEX_TRYLOCK_RECURSIVE:
@@ -45,15 +47,11 @@ static bool shrinker_lock(struct drm_i915_private *i915, bool *unlock)
case MUTEX_TRYLOCK_FAILED:
*unlock = false;
- preempt_disable();
- do {
- cpu_relax();
- if (mutex_trylock(&i915->drm.struct_mutex)) {
- *unlock = true;
- break;
- }
- } while (!need_resched());
- preempt_enable();
+ if (flags & I915_SHRINK_ACTIVE) {
+ mutex_lock_nested(&i915->drm.struct_mutex,
+ I915_MM_SHRINKER);
+ *unlock = true;
+ }
I just realized once oddity in the shrinker code which escaped me
before. It is the fact the call paths will call the shrinker_lock twice.
For instance i915_gem_shrinker_vmap and i915_gem_shrinker_scan. They
both first take lock with flags of zero, and then they call
i915_gem_shrink which takes the lock again, which obviously always
results in the recursive path to be taken.
I think we need to clean this up so it is easier to understand the code
before further tweaking, even if in this patch. For instance adding
I915_SHRINK_LOCKED would solve it.
shrinker_lock_uninterruptible is also funky in that it doesn't respect
the timeout in the waiting for idle phase.
Sounds reasonable?
Regards,
Tvrtko
return *unlock;
case MUTEX_TRYLOCK_SUCCESS:
@@ -160,7 +158,7 @@ i915_gem_shrink(struct drm_i915_private *i915,
unsigned long scanned = 0;
bool unlock;
- if (!shrinker_lock(i915, &unlock))
+ if (!shrinker_lock(i915, flags, &unlock))
return 0;
/*
@@ -357,7 +355,7 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
sc->nr_scanned = 0;
- if (!shrinker_lock(i915, &unlock))
+ if (!shrinker_lock(i915, 0, &unlock))
return SHRINK_STOP;
freed = i915_gem_shrink(i915,
@@ -397,7 +395,7 @@ shrinker_lock_uninterruptible(struct drm_i915_private *i915, bool *unlock,
do {
if (i915_gem_wait_for_idle(i915,
0, MAX_SCHEDULE_TIMEOUT) == 0 &&
- shrinker_lock(i915, unlock))
+ shrinker_lock(i915, 0, unlock))
break;
schedule_timeout_killable(1);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
