Re: [PATCH 06/16] drm/gem/shrinker: Wait before acquiring struct_mutex under oom

Chris Wilson
2016-08-04T06:52:22Z

On Thu, Aug 04, 2016 at 09:46:59AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> > We can now wait for the GPU (all engines) to become idle without
> > requiring the struct_mutex. Inside the shrinker, we need to currently
> > take the struct_mutex in order to purge objects and to purge the objects
> > we need the GPU to be idle - causing a stall whilst we hold the
> > struct_mutex. We can hide most of that stall by performing the wait
> > before taking the struct_mutex and only doing essential waits for
> > new rendering on objects to be freed.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_shrinker.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > index 1341cb55b6f1..43e53e419982 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > @@ -326,9 +326,14 @@ i915_gem_shrinker_lock_uninterruptible(struct drm_i915_private *dev_priv,
> >   unsigned long timeout = msecs_to_jiffies(timeout_ms) + 1;
> >
> >   while (!i915_gem_shrinker_lock(&dev_priv->drm, &slu->unlock)) {
> > + if (i915_gem_wait_for_idle(dev_priv) == 0 &&
>
> continue would be much cleaner here, to avoid repeating the lock
> calling line?

Yes. This was a small change, but adjusting the loop wasn't that much
bigger.

> Or how likely is it for engines to be idle but
> struct_mutex held for extended period?

Currently, quite possible. struct_mutex is the defacto bkl, we may be
causing memory pressure by swapping in gigabytes of an object on another
thread.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre